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

---
 doc/APIchanges      |  4 ++++
 libavutil/avutil.h  | 14 ++++++++++++++
 libavutil/utils.c   | 21 +++++++++++++++++++++
 libavutil/version.h |  2 +-
 4 files changed, 40 insertions(+), 1 deletion(-)

On 3/27/2017 4:51 AM, Clément Bœsch wrote:
> ---
>  doc/APIchanges      |  4 ++++
>  libavutil/avutil.h  | 14 ++++++++++++++
>  libavutil/utils.c   | 21 +++++++++++++++++++++
>  libavutil/version.h |  2 +-
>  4 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 6aaa9adceb..4736e3e6fc 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2015-08-28
>  
>  API changes, most recent first:
>  
> +2017-03-xx - xxxxxxx - lavu 55.52.100 - avutil.h
> +  add av_fourcc_make_string() function and av_4cc2str() macro to replace
> +  av_get_codec_tag_string() from lavc.
> +
>  2017-03-xx - xxxxxxx - lavc 57.85.101 - avcodec.h
>    vdpau hardware accelerated decoding now supports the new hwaccel API, which
>    can create the decoder context and allocate hardware frame automatically.
> diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> index e9aaa03722..98100fdcc5 100644
> --- a/libavutil/avutil.h
> +++ b/libavutil/avutil.h
> @@ -343,6 +343,20 @@ FILE *av_fopen_utf8(const char *path, const char *mode);
>   */
>  AVRational av_get_time_base_q(void);
>  
> +#define AV_FOURCC_MAX_STRING_SIZE 32
> +
> +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
> +
> +/**
> + * 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);

Maybe this could go in avstring.h instead.

> +
>  /**
>   * @}
>   * @}
> 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) == '_'))
> +
> +    for (i = 0; i < 4; i++) {
> +        len = snprintf(buf, buf_size,
> +                       TAG_PRINT(fourcc & 0xFF) ? "%c" : "[%d]", fourcc & 0xFF);
> +        buf       += len;
> +        buf_size   = buf_size > len ? buf_size - len : 0;
> +        fourcc   >>= 8;
> +    }
> +    return orig_buf;
> +}
> +
>  AVRational av_get_time_base_q(void)
>  {
>      return (AVRational){1, AV_TIME_BASE};
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 3c86c26638..d6d78e7dc2 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  55
> -#define LIBAVUTIL_VERSION_MINOR  51
> +#define LIBAVUTIL_VERSION_MINOR  52
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>

On Mon, Mar 27, 2017 at 11:16:24AM -0300, James Almer wrote:
> On 3/27/2017 4:51 AM, Clément Bœsch wrote:
> > ---
> >  doc/APIchanges      |  4 ++++
> >  libavutil/avutil.h  | 14 ++++++++++++++
> >  libavutil/utils.c   | 21 +++++++++++++++++++++
> >  libavutil/version.h |  2 +-
> >  4 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index 6aaa9adceb..4736e3e6fc 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -15,6 +15,10 @@ libavutil:     2015-08-28
> >  
> >  API changes, most recent first:
> >  
> > +2017-03-xx - xxxxxxx - lavu 55.52.100 - avutil.h
> > +  add av_fourcc_make_string() function and av_4cc2str() macro to replace
> > +  av_get_codec_tag_string() from lavc.
> > +
> >  2017-03-xx - xxxxxxx - lavc 57.85.101 - avcodec.h
> >    vdpau hardware accelerated decoding now supports the new hwaccel API, which
> >    can create the decoder context and allocate hardware frame automatically.
> > diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> > index e9aaa03722..98100fdcc5 100644
> > --- a/libavutil/avutil.h
> > +++ b/libavutil/avutil.h
> > @@ -343,6 +343,20 @@ FILE *av_fopen_utf8(const char *path, const char *mode);
> >   */
> >  AVRational av_get_time_base_q(void);
> >  
> > +#define AV_FOURCC_MAX_STRING_SIZE 32
> > +
> > +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
> > +
> > +/**
> > + * 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);
> 
> Maybe this could go in avstring.h instead.
> 

FourCC in strings? I feel like FourCC are more overall A/V specific than
string related. avstring is more about actually parsing string or craft
them from other strings.

Similarly, we didn't put timestamp string crafting in avstring,
av_ts2str() and its friends have their own header.

If more people feel like FourCC code should be there I'll move it,
otherwise I prefer if it stays here.

Hi Clément,

I think your idea of introducing this function and the convenience macro is good.

Below are some comments, feel free to ignore.


On 2017-03-27 09:51 +0200, Clément Bœsch wrote:
> ---
>  doc/APIchanges      |  4 ++++
>  libavutil/avutil.h  | 14 ++++++++++++++
>  libavutil/utils.c   | 21 +++++++++++++++++++++
>  libavutil/version.h |  2 +-
>  4 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 6aaa9adceb..4736e3e6fc 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2015-08-28
>  
>  API changes, most recent first:
>  
> +2017-03-xx - xxxxxxx - lavu 55.52.100 - avutil.h
> +  add av_fourcc_make_string() function and av_4cc2str() macro to replace
> +  av_get_codec_tag_string() from lavc.
> +
>  2017-03-xx - xxxxxxx - lavc 57.85.101 - avcodec.h
>    vdpau hardware accelerated decoding now supports the new hwaccel API, which
>    can create the decoder context and allocate hardware frame automatically.
> diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> index e9aaa03722..98100fdcc5 100644
> --- a/libavutil/avutil.h
> +++ b/libavutil/avutil.h
> @@ -343,6 +343,20 @@ FILE *av_fopen_utf8(const char *path, const char *mode);
>   */
>  AVRational av_get_time_base_q(void);
>  
> +#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.

> +
> +#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.

> +
> +/**
> + * 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.


> +
> +    for (i = 0; i < 4; i++) {
> +        len = snprintf(buf, buf_size,
> +                       TAG_PRINT(fourcc & 0xFF) ? "%c" : "[%d]", fourcc & 0xFF);
> +        buf       += len;
> +        buf_size   = buf_size > len ? buf_size - len : 0;
> +        fourcc   >>= 8;
> +    }
> +    return orig_buf;
> +}
> +
>  AVRational av_get_time_base_q(void)
>  {
>      return (AVRational){1, AV_TIME_BASE};
[...]

  Alexander

On Mon, Mar 27, 2017 at 09:51:54AM +0200, Clément Bœsch wrote:
> ---
>  doc/APIchanges      |  4 ++++
>  libavutil/avutil.h  | 14 ++++++++++++++
>  libavutil/utils.c   | 21 +++++++++++++++++++++
>  libavutil/version.h |  2 +-
>  4 files changed, 40 insertions(+), 1 deletion(-)
> 

I'll start pushing that patchset tomorrow if I see no objection.

[...]

2017-03-28 17:31 GMT+02:00 Clément Bœsch <u@pkh.me>:
> On Mon, Mar 27, 2017 at 09:51:54AM +0200, Clément Bœsch wrote:
>> ---
>>  doc/APIchanges      |  4 ++++
>>  libavutil/avutil.h  | 14 ++++++++++++++
>>  libavutil/utils.c   | 21 +++++++++++++++++++++
>>  libavutil/version.h |  2 +-
>>  4 files changed, 40 insertions(+), 1 deletion(-)
>>
>
> I'll start pushing that patchset tomorrow if I see no objection.

> +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)

Sorry, I really don't understand:
If the preferred name for this function is "av_4cc2str", why not name
it av_4cc2str()?

Carl Eugen

On Wed, Mar 29, 2017 at 09:31:57AM +0200, Carl Eugen Hoyos wrote:
> 2017-03-28 17:31 GMT+02:00 Clément Bœsch <u@pkh.me>:
> > On Mon, Mar 27, 2017 at 09:51:54AM +0200, Clément Bœsch wrote:
> >> ---
> >>  doc/APIchanges      |  4 ++++
> >>  libavutil/avutil.h  | 14 ++++++++++++++
> >>  libavutil/utils.c   | 21 +++++++++++++++++++++
> >>  libavutil/version.h |  2 +-
> >>  4 files changed, 40 insertions(+), 1 deletion(-)
> >>
> >
> > I'll start pushing that patchset tomorrow if I see no objection.
> 
> > +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
> 
> Sorry, I really don't understand:
> If the preferred name for this function is "av_4cc2str", why not name
> it av_4cc2str()?
> 

I don't understand your question: are you asking why there is a macro and
a function with different names?

BTW, I'm renaming the macro av_4cc2str() to av_fourcc2str() as it seems
more people prefer that name.

2017-03-29 9:43 GMT+02:00 Clément Bœsch <u@pkh.me>:
> On Wed, Mar 29, 2017 at 09:31:57AM +0200, Carl Eugen Hoyos wrote:
>> 2017-03-28 17:31 GMT+02:00 Clément Bœsch <u@pkh.me>:
>> > On Mon, Mar 27, 2017 at 09:51:54AM +0200, Clément Bœsch wrote:
>> >> ---
>> >>  doc/APIchanges      |  4 ++++
>> >>  libavutil/avutil.h  | 14 ++++++++++++++
>> >>  libavutil/utils.c   | 21 +++++++++++++++++++++
>> >>  libavutil/version.h |  2 +-
>> >>  4 files changed, 40 insertions(+), 1 deletion(-)
>> >>
>> >
>> > I'll start pushing that patchset tomorrow if I see no objection.
>>
>> > +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
>>
>> Sorry, I really don't understand:
>> If the preferred name for this function is "av_4cc2str", why not name
>> it av_4cc2str()?
>>
>
> I don't understand your question: are you asking why there is a macro
> and a function with different names?

Yes, why is a function with the preferred name not sufficient?

Carl Eugen

On Wed, Mar 29, 2017 at 09:52:51AM +0200, Carl Eugen Hoyos wrote:
> 2017-03-29 9:43 GMT+02:00 Clément Bœsch <u@pkh.me>:
> > On Wed, Mar 29, 2017 at 09:31:57AM +0200, Carl Eugen Hoyos wrote:
> >> 2017-03-28 17:31 GMT+02:00 Clément Bœsch <u@pkh.me>:
> >> > On Mon, Mar 27, 2017 at 09:51:54AM +0200, Clément Bœsch wrote:
> >> >> ---
> >> >>  doc/APIchanges      |  4 ++++
> >> >>  libavutil/avutil.h  | 14 ++++++++++++++
> >> >>  libavutil/utils.c   | 21 +++++++++++++++++++++
> >> >>  libavutil/version.h |  2 +-
> >> >>  4 files changed, 40 insertions(+), 1 deletion(-)
> >> >>
> >> >
> >> > I'll start pushing that patchset tomorrow if I see no objection.
> >>
> >> > +#define av_4cc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
> >>
> >> Sorry, I really don't understand:
> >> If the preferred name for this function is "av_4cc2str", why not name
> >> it av_4cc2str()?
> >>
> >
> > I don't understand your question: are you asking why there is a macro
> > and a function with different names?
> 
> Yes, why is a function with the preferred name not sufficient?
> 

Because we save the user the need to create a buffer thanks the compound
literal in the macro which can only exist in a macro. It's exactly like
av_ts2str().

Patchset pushed with av_fourcc2str() macro name.

[...]