[FFmpeg-devel] [PATCH] Yet another stab at RGB48 support

[Kostya]

On Tue, May 19, 2009 at 04:02:44AM +0200, Michael Niedermayer wrote:
> On Sat, May 16, 2009 at 09:24:13AM +0300, Kostya wrote:
[...]
> 
> > @@ -2054,6 +2064,8 @@
> >          uint8_t *srcPtr= src[0];
> >          if(srcFormat == PIX_FMT_RGB32_1 || srcFormat == PIX_FMT_BGR32_1)
> >              srcPtr += ALT32_CORR;
> > +        if(srcFormat == PIX_FMT_RGB48LE && !is16BPS(dstFormat))
> > +            srcPtr++; // we ignore low byte and can use the same functions then
> >  
> >          if (dstStride[0]*srcBpp == srcStride[0]*dstBpp && srcStride[0] > 0)
> >              conv(srcPtr, dst[0] + dstStride[0]*srcSliceY, srcSliceH*srcStride[0]);
> 
> same as what? please rewrite that comment in verbose mode
 
Same as for big-endian. Will change.
 
> > @@ -2172,6 +2184,8 @@
> 
> diffs with -p would be nice ...

noted 

> [...]
> 
> /------------------------------------------------\
> 
> > @@ -299,6 +301,8 @@
> >  #define is16BPS(x)      (           \
> >             (x)==PIX_FMT_GRAY16BE    \
> >          || (x)==PIX_FMT_GRAY16LE    \
> > +        || (x)==PIX_FMT_RGB48BE     \
> > +        || (x)==PIX_FMT_RGB48LE     \
> >          || (x)==PIX_FMT_YUV420PLE   \
> >          || (x)==PIX_FMT_YUV422PLE   \
> >          || (x)==PIX_FMT_YUV444PLE   \
> 
> \------------hunk ok-----------------------------/
> 
> 
> [...]
> 
> /------------------------------------------------\
> 
> >  #define isBGR(x)        (           \
> >             (x)==PIX_FMT_BGR32       \
> > @@ -376,6 +382,9 @@
> >  static inline int fmt_depth(int fmt)
> >  {
> >      switch(fmt) {
> > +        case PIX_FMT_RGB48BE:
> > +        case PIX_FMT_RGB48LE:
> > +            return 48;
> >          case PIX_FMT_BGRA:
> >          case PIX_FMT_ABGR:
> >          case PIX_FMT_RGBA:
> 
> \------------hunk ok-----------------------------/
 
I suppose adding it to sws_format_name() is also ok.
 
> [...]
> > +SwsFunc sws_hires_gray16rgbConv(SwsContext *c)
> > +{
> > +    const enum PixelFormat srcFormat = c->srcFormat;
> > +    const enum PixelFormat dstFormat = c->dstFormat;
> > +
> > +    if (isGray16(srcFormat) && (dstFormat == PIX_FMT_RGB48BE ||
> > +                                dstFormat == PIX_FMT_RGB48LE)) {
> > +        return gray16torgb48;
> > +    }
> > +    if (isGray16(dstFormat) && (srcFormat == PIX_FMT_RGB48BE ||
> > +                                srcFormat == PIX_FMT_RGB48LE)) {
> > +        if (dstFormat == PIX_FMT_GRAY16BE) {
> > +            if (srcFormat == PIX_FMT_RGB48BE)
> > +                return rgb48betogray16be;
> > +            else
> > +                return rgb48letogray16be;
> > +        } else {
> > +            if (srcFormat == PIX_FMT_RGB48BE)
> > +                return rgb48betogray16le;
> > +            else
> > +                return rgb48letogray16le;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> 
> the if / else looks messy why are this not 2 functions?
> 
> also dont forget to test the code with swscale-example, that is all new
> convertions should work and have small differences to the correct image
 
I've tested it on 48-bpp TIFF image, conversions to PNG and JPEG went
fine, will test a bit more.
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB