[FFmpeg-devel] [RFC] RGB32 pixfmts cleanup

Michael Niedermayer michaelni
Thu Mar 19 01:25:19 CET 2009


On Thu, Mar 19, 2009 at 01:12:46AM +0100, Stefano Sabatini wrote:
> On date Wednesday 2009-03-18 23:37:33 +0100, Michael Niedermayer encoded:
> > On Wed, Mar 18, 2009 at 10:58:20PM +0100, Stefano Sabatini wrote:
> > > On date Wednesday 2009-03-18 06:26:09 +0100, Michael Niedermayer encoded:
> > > > On Tue, Mar 17, 2009 at 11:56:30PM +0100, Stefano Sabatini wrote:
> > > > > On date Monday 2009-03-16 19:31:27 +0100, Michael Niedermayer encoded:
> > > > > > On Mon, Mar 16, 2009 at 09:31:56AM +0100, Stefano Sabatini wrote:
> > > > > > > On date Sunday 2009-03-15 20:39:08 +0100, Michael Niedermayer encoded:
> > > > > > > > On Sun, Mar 15, 2009 at 06:25:35PM +0100, Stefano Sabatini wrote:
> > > > > > > > > Hi all,
> > > > > > > > > 
> > > > > > > > > the attached (non-working) patch shows my idea for what regards a
> > > > > > > > > RGB32 pixfmts cleanup, I think this should somehow simplify the way we
> > > > > > > > > deal with such formats, also it delivers more meaningful names.
> > > > > > > > > 
> > > > > > > > > Main problem is that I'm duplicating the number of the RGB32 formats.
> > > > > > > > > 
> > > > > > > > > Comments on this are very welcome, if we decide to take this path I'd
> > > > > > > > > like to commit this before the end of the next week, thus taking
> > > > > > > > > advantage of the flux status of the API and avoiding to #ifversion.
> > > > > > > > [...]
> > > > > > > > > @@ -114,6 +110,15 @@
> > > > > > > > >      PIX_FMT_BGR555BE,  ///< packed BGR 5:5:5, 16bpp, (msb)1A 5B 5G 5R(lsb), big-endian, most significant bit to 1
> > > > > > > > >      PIX_FMT_BGR555LE,  ///< packed BGR 5:5:5, 16bpp, (msb)1A 5B 5G 5R(lsb), little-endian, most significant bit to 1
> > > > > > > > >  
> > > > > > > > > +    PIX_FMT_ARGB32BE,  ///< packed ARGB 8:8:8:8, 32bpp, (msb)8A 8R 8G 8B(lsb), big-endian
> > > > > > > > > +    PIX_FMT_ARGB32LE,  ///< packed ARGB 8:8:8:8, 32bpp, (msb)8A 8R 8G 8B(lsb), little-endian
> > > > > > > > > +    PIX_FMT_RGBA32BE,  ///< packed RGBA 8:8:8:8, 32bpp, (msb)8R 8G 8B 8A(lsb), big-endian
> > > > > > > > > +    PIX_FMT_RGBA32LE,  ///< packed RGBA 8:8:8:8, 32bpp, (msb)8R 8G 8B 8A(lsb), little-endian
> > > > > > > > > +    PIX_FMT_ABGR32BE,  ///< packed ABGR 8:8:8:8, 32bpp, (msb)8A 8B 8G 8R(lsb), big-endian
> > > > > > > > > +    PIX_FMT_ABGR32LE,  ///< packed ABGR 8:8:8:8, 32bpp, (msb)8A 8B 8G 8R(lsb), little-endian
> > > > > > > > > +    PIX_FMT_BGRA32BE,  ///< packed BGRA 8:8:8:8, 32bpp, (msb)8B 8G 8R 8A(lsb), big-endian
> > > > > > > > > +    PIX_FMT_BGRA32LE,  ///< packed BGRA 8:8:8:8, 32bpp, (msb)8B 8G 8R 8A(lsb), little-endian
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > this is sick
> > > > > > > 
> > > > > > > Please expand, this is not obvious to me (are you referring to the
> > > > > > > duplication?).
> > > > > > 
> > > > > > yes, the very same pixel format exists twice under 2 different enum
> > > > > > values
> > > > > 
> > > > > I see. Just a random (and maybe bad) idea:
> > > > > 
> > > > > enum PixelFormat {
> > > > > ...
> > > > >   PIX_FMT_RGBA32BE,
> > > > >   PIX_FMT_BGRA32BE,
> > > > >   PIX_FMT_ARGB32BE,
> > > > >   PIX_FMT_ABGR32BE,
> > > > > ...
> > > > > };
> > > > 
> > > > I am VERY VERY strongly against these names
> > > > 
> > > > ARGB
> > > > RGBA
> > > > ...
> > > > have perfectly clear and intuitiv definitions
> > > > 
> > > > and RGB48 LE & BE also have clear and totally different meaning from how
> > > > you use LE/BE here
> > > 
> > > Mmmh.. right.
> > > 
> > > So I'll just suggest to reorder them like in the attached patch.
> > 
> > moving ARGB, RGBA, ... into the enum and the RGB32 stuff in the define
> > should be better
> 
> Yes, much better, thanks!
> 
> Attached patch passes regressions test, bonus patch makes ffmpeg fail
> if the user provides a bogus pixel format.
> 
> Regards.
> -- 
> FFmpeg = Fostering Furious Monstrous Pacific Enigmatic Generator

> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 18038)
> +++ ffmpeg.c	(working copy)
> @@ -2536,9 +2536,13 @@
>  
>  static void opt_frame_pix_fmt(const char *arg)
>  {
> -    if (strcmp(arg, "list"))
> +    if (strcmp(arg, "list")) {
>          frame_pix_fmt = avcodec_get_pix_fmt(arg);
> +        if (frame_pix_fmt == PIX_FMT_NONE) {
> +            fprintf(stderr, "Unknown pixel format requested: %s\n", arg);
> +            av_exit(1);
> +        }
> -    else {
> +    } else {
>          list_fmts(avcodec_pix_fmt_string, PIX_FMT_NB);
>          av_exit(0);
>      }

ok


[..]
> @@ -91,8 +89,10 @@
>      PIX_FMT_NV12,      ///< planar YUV 4:2:0, 12bpp, 1 plane for Y and 1 for UV
>      PIX_FMT_NV21,      ///< as above, but U and V bytes are swapped
>  
> -    PIX_FMT_RGB32_1,   ///< packed RGB 8:8:8, 32bpp, (msb)8R 8G 8B 8A(lsb), in CPU endianness
> -    PIX_FMT_BGR32_1,   ///< packed RGB 8:8:8, 32bpp, (msb)8B 8G 8R 8A(lsb), in CPU endianness
> +    PIX_FMT_ARGB,      ///< packed RGB 8:8:8, 32bpp, (msb)8A 8R 8G 8B(lsb), in CPU endianness
> +    PIX_FMT_RGBA,      ///< packed RGB 8:8:8, 32bpp, (msb)8R 8G 8B 8A(lsb), in CPU endianness
> +    PIX_FMT_ABGR,      ///< packed RGB 8:8:8, 32bpp, (msb)8A 8B 8G 8R(lsb), in CPU endianness
> +    PIX_FMT_BGRA,      ///< packed RGB 8:8:8, 32bpp, (msb)8B 8G 8R 8A(lsb), in CPU endianness
>  
>      PIX_FMT_GRAY16BE,  ///<        Y        , 16bpp, big-endian
>      PIX_FMT_GRAY16LE,  ///<        Y        , 16bpp, little-endian

the description is wrong, these are not in cpu endianness

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090319/6798d24a/attachment.pgp>



More information about the ffmpeg-devel mailing list