[FFmpeg-cvslog] ffprobe: add pixel format flags output

[Alexander Strasser]

Hi!

On 2014-10-07 23:28 +0200, Tobias Rapp wrote:
> ffmpeg | branch: master | Tobias Rapp <t.rapp at noa-audio.com> | Mon Sep 15 17:15:17 2014 +0200| [b36b2c89dfd7540c8b4a041fbe702d02a8f04f9e] | committer: Michael Niedermayer
> 
> ffprobe: add pixel format flags output
> 
> Adds output of pixel format flags to ffprobe -show_pixel_formats option.
> 
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> 
> > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=b36b2c89dfd7540c8b4a041fbe702d02a8f04f9e
> ---
> 
>  doc/ffprobe.xsd |   15 +++++++++++++++
>  ffprobe.c       |   23 ++++++++++++++++++++++-
>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
[...]
> diff --git a/ffprobe.c b/ffprobe.c
> index 41667f1..c0f9b84 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
> @@ -67,6 +67,7 @@ static int do_show_data    = 0;
[...]
>  
> +#define PRINT_PIX_FMT_FLAG(flagname, name)                                \
> +    do {                                                                  \
> +        print_int(name, !!(pixdesc->flags & AV_PIX_FMT_FLAG_##flagname)); \
> +    } while (0)
> +
>  static void ffprobe_show_pixel_formats(WriterContext *w)
>  {
>      const AVPixFmtDescriptor *pixdesc = NULL;
> @@ -2576,6 +2584,18 @@ static void ffprobe_show_pixel_formats(WriterContext *w)
>          n = av_get_bits_per_pixel(pixdesc);
>          if (n) print_int    ("bits_per_pixel", n);
>          else   print_str_opt("bits_per_pixel", "N/A");
> +        if (do_show_pixel_format_flags) {
> +            writer_print_section_header(w, SECTION_ID_PIXEL_FORMAT_FLAGS);
> +            PRINT_PIX_FMT_FLAG(BE,        "big_endian");
> +            PRINT_PIX_FMT_FLAG(PAL,       "palette");
> +            PRINT_PIX_FMT_FLAG(BITSTREAM, "bitstream");
> +            PRINT_PIX_FMT_FLAG(HWACCEL,   "hwaccel");
> +            PRINT_PIX_FMT_FLAG(PLANAR,    "planar");
> +            PRINT_PIX_FMT_FLAG(RGB,       "rgb");
> +            PRINT_PIX_FMT_FLAG(PSEUDOPAL, "pseudopal");
> +            PRINT_PIX_FMT_FLAG(ALPHA,     "alpha");
> +            writer_print_section_footer(w);
> +        }
>          writer_print_section_footer(w);
>      }
>      writer_print_section_footer(w);

  I do not know if we do this in other places too, but when I read
this now I think it is no good idea too split public names of the
API like AV_PIX_FMT_FLAG_* into parts.

  It hurts searchability of the code base. Would we e.g. give a new
name to AV_PIX_FMT_FLAG_PAL like AV_PIX_FMT_FLAG_PALETTE we would
certainly miss this occurrence when doing search & replace.

  I think it's no big deal and easy to change in this case:

-#define PRINT_PIX_FMT_FLAG(flagname, name)                                \
+#define PRINT_PIX_FMT_FLAG(flag, name)                                    \
     do {                                                                  \
         print_int(name, !!(pixdesc->flags & (flag))); \
     } while (0)

then replace the partial flags with the full name where the macro
is used.

  Or maybe even better, avoid the macro completely:

-            PRINT_PIX_FMT_FLAG(BE,        "big_endian");
+            print_int("big_endian", !!(pixdesc->flags & AV_PIX_FMT_FLAG_BE));

  I am bringing this up here because I want to know what other
developers think before sending patches for disputable changes.
Or maybe the author wants to change it himself.

[...]

If you got here, thank you for your patient reading ;)
  Alexander
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-cvslog/attachments/20141010/698afef0/attachment.asc>