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

[Alexander Strasser]

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