On 2017-03-28 10:19 +0200, Clément Bœsch wrote:
> On Tue, Mar 28, 2017 at 12:47:39AM +0200, Alexander Strasser wrote:
> [...]
> > > +#define AV_FOURCC_MAX_STRING_SIZE 32
> >
> > Should be fine, though I don't know how you came up with this max size in
> > particular.
> >
>
> IIRC the actual maximum is 21 characters, I just took the closest superior
> power of two. Probably just like we did for AV_TS_MAX_STRING_SIZE.
OK
> > > +
> > > +#define av_4cc2str(fourcc)
> > > av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
> >
> > Did your write it as 4cc2str to make the name shorter?
> > I guess fourcc2str is more readable and more consistent.
> >
>
> This is a shorthand, so yeah I wanted to keep it short. If you replace
> "4cc" with "fourcc", why not do that for the '2' as well? Writing "four"
> but keeping '2' was kind of weird to me :)
Well, to expand on my point a bit. The 2 in these kind of functions works
like a delimiter to me e.g. "a2b"; easy to tokenize. When looking at "4a2b",
it is a bit harder. I know the example is a bit more extreme.
The other thing is that 4 is also sometimes used as meaning "for", similar
to the usage of 2 as meaning "to".
TL;DR: I am fine with the name you chose, just wanted to tell my impression.
Regarding the follow-up patches renaming would also cause a fair bit of labour
to you. So I can totally understand if you keep the name.
> > > +
> > > +/**
> > > + * Fill the provided buffer with a string containing a FourCC
> > > (four-character
> > > + * code) representation.
> > > + *
> > > + * @param buf a buffer with size in bytes of at least
> > > AV_FOURCC_MAX_STRING_SIZE
> > > + * @param fourcc the fourcc to represent
> > > + * @return the buffer in input
> > > + */
> > > +char *av_fourcc_make_string(char *buf, uint32_t fourcc);
> > > +
> > > /**
> > > * @}
> > > * @}
> > > diff --git a/libavutil/utils.c b/libavutil/utils.c
> > > index 36e4dd5fdb..ba557b9252 100644
> > > --- a/libavutil/utils.c
> > > +++ b/libavutil/utils.c
> > > @@ -121,6 +121,27 @@ unsigned av_int_list_length_for_size(unsigned elsize,
> > > return i;
> > > }
> > >
> > > +char *av_fourcc_make_string(char *buf, uint32_t fourcc)
> > > +{
> > > + int i, len;
> > > + char *orig_buf = buf;
> > > + size_t buf_size = AV_FOURCC_MAX_STRING_SIZE;
> > > +
> > > +#define TAG_PRINT(x) \
> > > + (((x) >= '0' && (x) <= '9') || \
> > > + ((x) >= 'a' && (x) <= 'z') || ((x) >= 'A' && (x) <= 'Z') || \
> > > + ((x) == '.' || (x) == ' ' || (x) == '-' || (x) == '_'))
> >
> > You could spare this macro, by using a temporary char for the character and
> > an int to indicate if it is printable in the loop below. Would probably be
> > 1 or 2 lines longer.
> >
> > You might want to at least rename TAG_PRINT to IS_PRINTABLE or
> > IS_PRINTABLE_FOURCC_CHAR or similar as TAG_PRINT is a rather confusing name.
> >
> > Also if the macro is only introduced for use this function, #undef could be
> > used, but I don't think we really do it anywhere else.
> >
>
> Yeah well I just took the existing code in libavcodec, so I didn't want to
> change it too much.
>
> But you're right on all your comments so I just adjusted the function. See
> patch attached.
I will comment on the patch in response to Michael's comment.
Alexander
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
