[FFmpeg-devel] PixFmtInfo cleanup
Michael Niedermayer
michaelni
Mon Feb 23 02:26:16 CET 2009
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
3. test that it is functional by implementing a generic function reading
samples
int read_sample(uint8_t *data[4], int linesize[4], pix_fmt_descriptor *desc, int x, int y, int c){
int par= desc->param[c];
if(desc->flags & PIX_FMT_PACKED_BITS){
int val;
int mask= (1<<(par>>4))-1;
if (desc->flags & PIX_FMT_16BE) val= AV_RB16(data[0]+y*linesize[0] + 2*x);
else if(desc->flags & PIX_FMT_16LE) val= AV_RL16(data[0]+y*linesize[0] + 2*x);
else val= data[0][y*linesize[0] + x];
val = (val>>(par&15)) & mask;
if(desc->flags & PIX_FMT_PAL)
val= data[1][4*val + c];
return val;
}else{
int plane= par>>6;
int index= ((par>>3)&7) + x*(par&7);
if (desc->flags & PIX_FMT_16BE) return AV_RB16(data[plane]+y*linesize[plane]+2*index);
else if(desc->flags & PIX_FMT_16LE) return AV_RL16(data[plane]+y*linesize[plane]+2*index);
else return data[plane][y*linesize[plane]+ index];
}
}
ive already suggested something similar a while ago but nothing was commited
(if someone has a link to previous discussions, that might also be usefull)
either way i think that such a generic pix_fmt_descriptor struct should be
added to lavu or lavc and exported
The goal of that should be to allow generic and not too inefficient reading
and writing of pixel formats.
The actual table of descriptors could be in lavc or lavu, it actually makes
more sense to put the table in lavc ...
comments welcome
also please tell me if you want to work on this or not so it doesnt silently
die like the last time it was discussed...
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20090223/b37d1c4c/attachment.pgp>
More information about the ffmpeg-devel
mailing list