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

Michael Niedermayer michaelni
Thu May 21 16:37:09 CEST 2009


On Wed, May 20, 2009 at 06:05:28PM +0300, Kostya wrote:
> On Wed, May 20, 2009 at 01:07:36AM +0200, Michael Niedermayer wrote:
> > On Tue, May 19, 2009 at 08:32:53PM +0300, Kostya wrote:
> > > 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:
> [...]
> > >  
> > > I've tested it on 48-bpp TIFF image, conversions to PNG and JPEG went
> > > fine, will test a bit more.
> > 
> > you can test all ~1000 cases by hand too if you dont want to use
> > swscale_example, but testing just 2 cases is not enough
>  
> Okay,  here it is - tested, seems to work fine.

have you also tried swscale_example with SWS_ACCURATE_RND / SWS_BITEXACT?


> 
> > [...]
> > 
> > -- 
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

>  Makefile           |    2 
>  hires.c            |  270 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  swscale-example.c  |    4 
>  swscale.c          |   44 ++++++++
>  swscale_internal.h |    4 
>  swscale_template.c |   50 +++++++++
>  yuv2rgb.c          |   39 +++++++
>  7 files changed, 409 insertions(+), 4 deletions(-)
> 740a8362d57945a232868f0e69042aff4b8f8ade  48bpp-new.patch
> Index: yuv2rgb.c
> ===================================================================
> --- yuv2rgb.c	(revision 29317)
> +++ yuv2rgb.c	(working copy)
> @@ -80,6 +80,16 @@ const int32_t ff_yuv2rgb_coeffs[8][4] = {
>      Y = ysrc[2*i+1-o];                                  \
>      dst[2*i+1] = r[Y] + g[Y] + b[Y] + (asrc[2*i+1]<<s);
>  
> +#define PUTRGB48(dst,src,i)                   \
> +    Y = src[2*i];                             \

> +    dst[12*i+ 0] = r[Y]; dst[12*i+ 1] = r[Y]; \

dst[12*i+ 0] = dst[12*i+ 1] = r[Y];


[...]
> @@ -1996,6 +2016,8 @@ static int pal2rgbWrapper(SwsContext *c, uint8_t*
>      return srcSliceH;
>  }
>  
> +extern SwsRGBFunc sws_hires_getRGBFunc(SwsContext *c);
> +
>  /* {RGB,BGR}{15,16,24,32,32_1} -> {RGB,BGR}{15,16,24,32} */
>  static int rgb2rgbWrapper(SwsContext *c, uint8_t* src[], int srcStride[], int srcSliceY,
>                            int srcSliceH, uint8_t* dst[], int dstStride[]){

this belongs in a header


> @@ -2008,7 +2030,9 @@ static int rgb2rgbWrapper(SwsContext *c, uint8_t*
>      void (*conv)(const uint8_t *src, uint8_t *dst, long src_size)=NULL;
>  
>      /* BGR -> BGR */
> -    if (  (isBGR(srcFormat) && isBGR(dstFormat))
> +    if (srcBpp > 4 || dstBpp > 4) {
> +        conv = sws_hires_getRGBFunc(c);
> +    } else if (  (isBGR(srcFormat) && isBGR(dstFormat))
>         || (isRGB(srcFormat) && isRGB(dstFormat))){
>          switch(srcId | (dstId<<4)){
>          case 0x34: conv= rgb16to15; break;

> @@ -2058,6 +2082,8 @@ static int rgb2rgbWrapper(SwsContext *c, uint8_t*
>          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++; // in this case conversion code takes only high bytes
>  
>          if (dstStride[0]*srcBpp == srcStride[0]*dstBpp && srcStride[0] > 0)
>              conv(srcPtr, dst[0] + dstStride[0]*srcSliceY, srcSliceH*srcStride[0]);

can that condition even be true?
anyway please remove sws_hires_getRGBFunc() its a duplicate of
rgb2rgbWrapper()


> @@ -2176,6 +2202,8 @@ static int planarCopy(SwsContext *c, uint8_t* src[
>              fillPlane(dst[plane], dstStride[plane], length, height, y, (plane==3) ? 255 : 128);
>          }else
>          {
> +            if (isRGB(c->srcFormat))
> +                length *= isALPHA(c->srcFormat) ? 4 : 3;
>              if(is16BPS(c->srcFormat) && !is16BPS(c->dstFormat)){
>                  if (!isBE(c->srcFormat)) srcPtr++;
>                  for (i=0; i<height; i++){

iam sorry but that goes too far
some people say sws is a mess or at least undocumented and thus hard to
understand but what you do here is beyond the worst code in sws.

The function is called planarCopy() that implicates planar data, and you
simply use it to copy packed pixel formats and randomly hack it so it
halfway works

I think it should go without saying that the name of a function has to match
what it does and that all non obvious tricks must be documented.
Also if for whatever reason you wish to merge packed and planarCopy that
would be a seperate patch with a few words of why the merge is a good idea/
needed

similarly yuv2rgb48 should be a seperate patch and the basic rgb48 support
should be seperate from the unscaled special converteres for rgb48

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- 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/20090521/69b2a316/attachment.pgp>



More information about the ffmpeg-devel mailing list