Hi! On 2014-10-07 23:28 +0200, Tobias Rapp wrote: > ffmpeg | branch: master | Tobias Rapp <[email protected]> | 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 <[email protected]> > > > 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
pgpqZ9j770zMB.pgp
Description: PGP signature
_______________________________________________ ffmpeg-cvslog mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-cvslog
