[FFmpeg-devel] PixFmtInfo cleanup

[Stefano Sabatini]

On date Tuesday 2009-02-24 02:13:08 +0100, Michael Niedermayer encoded:
> On Sun, Feb 22, 2009 at 06:40:18PM -0800, Baptiste Coudurier wrote:
> > On 2/22/2009 5:26 PM, Michael Niedermayer wrote:
> > > On Sun, Feb 22, 2009 at 11:30:50PM +0100, Stefano Sabatini wrote:
> > >> On date Thursday 2009-02-19 18:52:23 +0100, Michael Niedermayer encoded:
> > >>> On Thu, Feb 19, 2009 at 06:10:50PM +0100, Vitor Sessak wrote:
> > >>>> Michael Niedermayer wrote:
> > >>>>> On Thu, Feb 19, 2009 at 01:19:22AM +0100, Stefano Sabatini wrote:
> > >>>>>> On date Thursday 2009-02-19 00:20:23 +0100, Michael Niedermayer encoded:
> > >>>>>>> On Thu, Feb 19, 2009 at 12:03:26AM +0100, Stefano Sabatini wrote:
> > >>>>>>>> On date Sunday 2009-02-15 20:58:35 +0100, Vitor Sessak encoded:
> > >>>>>>>>> Stefano Sabatini wrote:
> > >> [...]
> > >>>>>>>> My idea was to move the macros to lavu, redefine them something like:
> > >>>>>>>> AV_PIX_FMT_IS_PLANAR_YUV()
> > >>>>>>>> AV_PIX_FMT_IS_YUV()
> > >>>>>>>> AV_PIX_FMT_IS_GRAY()
> > >>>>>>>> ...
> > >>>>>>>>
> > >>>>>>>> and redefine the current libsws macros like:
> > >>>>>>>> #define isPlanarYUV(x) AV_PIX_FMT_IS_PLANAR_YUV(x)
> > >>>>>>>>
> > >>>>>>>> swscale_internal.h already depends on libavutil/avutil.h.
> > >>>>>>>>
> > >>>>>>>> Would be this a acceptable?
> > >>>>>>> first the macros in sws are inefficiently implemented,
> > >>>>>>> an array so that
> > >>>>>>>
> > >>>>>>> AV_PIX_FMT_IS_YUV(x) (array[x]&0x01)
> > >>>> Why using macros at all and not inline functions?
> > >>> because the call overhead exceeds the amount of code and i dont trust
> > >>> the compiler
> > >>>>>>> would be a good idea, also see pix_fmt_info
> > >>>>>> Yes, I'm thinking about to move PixFmtInfo to lavu too.
> > >>>> I like this idea. There are a lot of filters that could use data from 
> > >>>> PixFmtInfo.
> > >>>>
> > >>>>>>> if its well and clean implemenetd lavu is an option also sws should n that
> > >>>>>>> case use lavus variant directly and not use wraper macros
> > >>>>>> First step creates a pix_fmt.h header (PixFmtInfo would then be added
> > >>>>>> to libavutil/pix_fmt.c)
> > >>>>> PixFmtInfo as is is too bloated, it requires cleanup _first_
> > >>>> What is bloated/ugly about it? The only thing I see that could be 
> > >>>> improved is putting all the FF_COLOR_XXX and FF_PIXEL_XXX in an enum each...
> > >>> 12 bytes per pix_fmt for roughly 2 byte of actual information
> > >> Well, I can think of this kind of compression:
> > >>
> > >> #define FF_COLOR_RGB          0 ///< RGB color space
> > >> #define FF_COLOR_GRAY         1 ///< gray color space
> > >> #define FF_COLOR_YUV          2 ///< YUV color space. 16 <= Y <= 235, 16 <= U, V <= 240
> > >> #define FF_COLOR_YUV_JPEG     3 ///< YUV color space. 0 <= Y <= 255, 0 <= U, V <= 255
> > >>
> > >> #define FF_PIXEL_PLANAR       4 ///< each channel has one component in AVPicture
> > >> #define FF_PIXEL_PACKED       5 ///< only one components containing all the channels
> > >> #define FF_PIXEL_PALETTE      6 ///< one components containing indexes for a palette
> > >>
> > >> #define FF_PIXEL_HAS_ALPHA    7 ///< true if alpha can be specified
> > >>
> > >> typedef struct PixFmtInfo {
> > >>     const char *name;
> > >>     uint8_t nb_channels;     /**< number of channels (including alpha) */
> > >>     uint8_t flags;
> > >>     uint8_t x_chroma_shift;  /**< X chroma subsampling factor is 2 ^ shift */
> > >>     uint8_t y_chroma_shift;  /**< Y chroma subsampling factor is 2 ^ shift */
> > >>     uint8_t depth;           /**< bit depth of the color components */
> > >> } PixFmtInfo;
> > >>
> > >> which saves two bytes, but I'm not sure it's a good idea, after all
> > >> the color/pixel type are mutually exclusive.
> > >>
> > >> Similarly we may pack the
> > >> depth/x_chroma_shift/y_chroma_shift/nb_channels into another flag var,
> > >> but still cannot see the point of such a move, since it would make the
> > >> code more complicated just for a little gain in the memory footprint.
> > > 
> > > 
> > > nb_channels, is what ?
> > > its 4 for rgba that has 4 components and uses just data[0]
> > > its 2 for nv12 that has 3 components and uses data[0/1]
> > > its 4 for pal that uses data[0/1]
> > > this is not consistent. It has to be fixed
> > > 
> > > then depth is 5 for rgb565, what is this supposed to mean?!
> > > depth needs a clear definition and it needs to be usefull for something
> > > else it should be removed.
> > > 
> > > next comes the colorspace (RGB, YUV, JPEG YUV, wait there are more YUV
> > > spaces than these 2 ...) and the pixel packing mixed together ...
> > > 
> > > maybe you now see what problem i have with it ...
> > > its just a collection of random hacks to keep imgconvert from falling
> > > apart, not some information that can be used or that is extendible
> > > i dont want this table exported like that ...
> > > 
> > > what should be done:
> > > 1. define pix fmt
> > >    Taking what is closest to the current code, pix_fmt specifies how bits
> > >    from pixels are packed into up to 4 planes. With this definition the
> > >    jpeg yuv formats are practically deprecated. yuv type should be
> > >    specified seperate of the pix_fmt which is alot more flexible than just
> > >    supporting yuvj, keep in mind sws supports these other yuv types in
> > >    many cases we just have no means to transmit the info from decoder to
> > >    sws
> > > 2. specify a struct describing pix fmt
> > > 
> > > struct pix_fmt_descriptor{
> > >     uint8_t nb_channels;        ///< The number of components each pixel has, (1-4)
> > >     uint8_t log2_chroma_w;      ///< chroma_width = -((-luma_width )>>log2_chroma_w)
> > >     uint8_t log2_chroma_h;      ///< chroma_height= -((-luma_height)>>log2_chroma_h)
> > >     uint8_t param[4];           ///< parameters that describes how pixels are packed
> > >     uint8_t flags;
> > > }
> > > 
> > > #define PIX_FMT_16BE        1
> > > #define PIX_FMT_16LE        2
> > > #define PIX_FMT_PAL         4
> > > #define PIX_FMT_PACKED_BITS 8
> > 
> > Looks nice, can you please describe what would be PAL 
> 
> in svn

Nice, just one maybe silly question, why all these minus1/plus1?

And simple patch attached.
-- 
FFmpeg = Fast and Frightening Meaningless Powerful Embarassing God
-------------- next part --------------
A non-text attachment was scrubbed...
Name: expand-pixdesc-lsb.patch
Type: text/x-diff
Size: 852 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090304/cd8b4050/attachment.patch>