[FFmpeg-devel] [RFC] RGB32 pixfmts cleanup

[Stefano Sabatini]

On date Sunday 2009-03-22 22:44:58 +0100, Michael Niedermayer encoded:
> On Sun, Mar 22, 2009 at 07:09:21PM +0100, Stefano Sabatini wrote:
> > On date Sunday 2009-03-22 18:33:37 +0100, Michael Niedermayer encoded:
> > > On Sun, Mar 22, 2009 at 05:56:29PM +0100, Stefano Sabatini wrote:
> > > > On date Sunday 2009-03-22 16:45:50 +0100, Michael Niedermayer encoded:
> > > > > On Sun, Mar 22, 2009 at 04:20:38PM +0100, Stefano Sabatini wrote:
> > > > > > On date Thursday 2009-03-19 01:25:19 +0100, Michael Niedermayer encoded:
> > > > > > > On Thu, Mar 19, 2009 at 01:12:46AM +0100, Stefano Sabatini wrote:
> > > > > > [...]
> > > > > > > [..]
> > > > > > > > @@ -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
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > checking again the patch, the define change will broke regression,
> > > > > > since "rgb32", "rgb32_1" and friends have different meanings according
> > > > > > to the endianess of the system.
> > > > > > 
> > > > > > So we have:
> > > > > > 
> > > > > > name      |   BE   |  LE
> > > > > > ----------+--------+------
> > > > > > "rgb32"   |  ARGB  | BGRA     
> > > > > > "rgb32_1" |  RGBA  | ABGR
> > > > > > "bgr32"   |  ABGR  | RGBA
> > > > > > "bgr32_1" |  BGRA  | ARGB
> > > > > > 
> > > > > > That is a test for the format "rgba" will work differently in LE and
> > > > > > BE systems.
> > > > > > 
> > > > > > A possible solution would be to extend the avcodec_get_pix_fmt() so
> > > > > > that it will recognize also the "rgb32", "rgb32_1", etc. names, and
> > > > > > will return the right pixel format according to the previous table.
> > > > > 
> > > > > its ok to make t recognize "rgb32" "bgr32"
> > > > >
> > > > > > 
> > > > > > A cleaner solution may be to extend the PixFmtInfo/AVPixFmtDecriptor
> > > > > > to support also one or more aliases for each pixel format name.
> > > > > > Then we could define the various alias for "rgba", "bgra", etc
> > > > > > according to the system endianess.
> > > > > 
> > > > > iam against ading a alias field that will be unused by 98% of the
> > > > > entries and will complicate all code
> > > > 
> > > > Check the attached patches.
> > > > 
> > > > But I continue to prefer the previous solution, while it requires more
> > > > space, it's simpler and more extensible, also with it would be easy
> > > > to automatically expose the aliases (for example in
> > > > avcodec_get_pix_fmt_string), which doesn't look feasible with the second
> > > > solution.
> > > 
> > > id like to see the special case hacks droped not its extension and spread
> > > made easier
> > > 
> > > > Regards.
> > > > -- 
> > > > FFmpeg = Fostering Faithful Mysterious Programmable EniGma
> > > 
> > > > Index: ffmpeg/libavcodec/imgconvert.c
> > > > ===================================================================
> > > > --- ffmpeg.orig/libavcodec/imgconvert.c	2009-03-22 17:14:31.000000000 +0100
> > > > +++ ffmpeg/libavcodec/imgconvert.c	2009-03-22 17:53:30.000000000 +0100
> > > > @@ -499,14 +499,31 @@
> > > >      return PIX_FMT_NONE;
> > > >  }
> > > >  
> > > > +struct {
> > > > +    const char *alias;
> > > > +    const char *name;
> > > > +} pix_fmt_aliases[] = {
> > > > +};
> > > > +
> > > 
> > > no
> > > i agree to seeing exactly 2 if() with 2 strcmp added
> > 
> > Check again.
> 
> ok

Applied.

Two weeks API instability interval expired, I think it's time to
declare the hunting season closed ;-).

Attached a last refactoring patch.

Regards.
-- 
FFmpeg = Foolish and Fantastic Most Portable Elegant Governor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pixfmt-factorize-pix-fmt-ne.patch
Type: text/x-diff
Size: 1646 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090323/829d6250/attachment.patch>