[FFmpeg-devel] [PATCH 01/10] lavu: add av_fourcc_make_string() and av_4cc2str()

[Alexander Strasser]

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