[FFmpeg-devel] PixFmtInfo cleanup

[Michael Niedermayer]

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


> and PACKED_BITS ?

doesnt exist anymore


> 
> Also where would "bits per component" be stored ? in param ?

comp[].depth

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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/20090224/59092871/attachment.pgp>