[FFmpeg-devel] PixFmtInfo cleanup

[Stefano Sabatini]

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.

Regards.
-- 
FFmpeg = Faithful & Fundamental Magic Puristic Elected Generator