[FFmpeg-devel] [PATCH] define AVPixelFormat aliases as enumerators instead of macros

[wm4]

On Fri, 15 Jan 2016 10:39:59 -0800
Richard Smith <richard at metafoo.co.uk> wrote:

> On Fri Jan 15 08:51:07 CET 2016 wm4 <nfxjfg at googlemail.com> wrote;
> > On Thu, 14 Jan 2016 13:58:14 -0800 Richard Smith <richard at metafoo.co.uk> wrote:  
> > > libavutil/pixfmt.h defines a collection of endian-specific pixel formats as
> > > macros. These macro names can cause conflicts with external projects that
> > > use those identifiers for their own purposes. Here's a patch to define
> > > these aliases as enumerators instead of macros, please consider merging:
> > >
> > >
> > > https://github.com/zygoloid/FFmpeg/commit/c20a0e2e66e52c45b9193bc750165b7ecf7f3ca4
> > >
> > > (Note that AV_PIX_FMT_Y400A was already defined as an enumerator in the
> > > PixelFormat enumeration, so I deleted its (no-op) macro entirely.)  
> >
> > API users might check for the existence of such pixfmts with #ifdef,  
> 
> That would be a very odd thing for them to do, as most pixfmts do not
> have #defines.
> 
> > and I don't understand the reasoning behind your patch. Why would
> > external projects redefine these macros?  
> 
> The project in question has its own enumeration:
> 
> namespace MyProject {
>   enum PixelFormatToUse {
>     // ... some other values ...
>     AV_PIX_FMT_RGB32, // use ffmpeg's AV_PIX_FMT_RGB32
>     // ...
>   };
> }

Then maybe it shouldn't do that, or if it does, this source file
shouldn't include any ffmpeg related headers. This is just asking for
trouble big-time. Use a different prefix if you really want to do this.
FFmpeg uses preprocessor defines for a lot of things, and you just have
to expect that FFmpeg will add macros prefixed with AV_ as it pleases.

> The names are intentionally chosen to be in 1-1 correspondence with
> ffmpeg's names. But ffmpeg's macro sometimes renames this project's
> enumerator, depending on whether its header is included before that
> file.

This is not a question of what should be, it's question of
compatibility. We try not to break downstream projects unnecessarily.

I'd just suggest defining aliases with names that do not violate
ffmpeg's namespace.

If you want to do this change, you IMHO need to keep the defines under a
compatibility ifdef, that will remain active until the next major
library bump. (Major bumps are when we change the API/ABI in
incompatible ways. Look e.g. into libavutil/version.h for a bunch of
APIs that will be removed or changed on the next bump.)

I don't know if others shares my opinion whether compatibility is
needed here.