[FFmpeg-devel] PixFmtInfo cleanup

[Baptiste Coudurier]

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 and PACKED_BITS ?

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

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org