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

Michael Niedermayer michaelni
Fri May 22 03:08:56 CEST 2009


On Thu, May 21, 2009 at 08:51:44PM +0300, Kostya wrote:
> On Thu, May 21, 2009 at 04:37:09PM +0200, Michael Niedermayer wrote:
> > 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?
>  
> I've tried it with all the flags it uses by default, error seems to be
> low (i.e. one-digit number).

all cases must work not only the default flags


> 
> [...]
> > 
> > > @@ -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
> 
> What about this patch for RGB48 -> YUV conversion?
> Also YUV -> RGB48 is easy too but rgb2rgb stuff is PITA to integrate
> in clean and logical way and will require a great deal more time to do
> it properly.
>  
> > [...]
> > -- 
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

>  swscale.c          |    2 +
>  swscale_internal.h |    4 ++-
>  swscale_template.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 8b0dea176685da1b215ef85dafde3594459d0c5c  48bpp-in.patch
> Index: swscale.c
> ===================================================================
> --- swscale.c	(revision 29318)
> +++ swscale.c	(working copy)
> @@ -108,6 +108,8 @@ unsigned swscale_version(void)
>          || (x)==PIX_FMT_YUVA420P    \
>          || (x)==PIX_FMT_YUYV422     \
>          || (x)==PIX_FMT_UYVY422     \
> +        || (x)==PIX_FMT_RGB48BE     \
> +        || (x)==PIX_FMT_RGB48LE     \
>          || (x)==PIX_FMT_RGB32       \
>          || (x)==PIX_FMT_RGB32_1     \
>          || (x)==PIX_FMT_BGR24       \
> Index: swscale_internal.h
> ===================================================================
> --- swscale_internal.h	(revision 29318)
> +++ swscale_internal.h	(working copy)
> @@ -344,7 +344,9 @@ const char *sws_format_name(int format);
>          || (x)==PIX_FMT_GRAY16LE    \
>      )
>  #define isRGB(x)        (           \
> -           (x)==PIX_FMT_RGB32       \
> +           (x)==PIX_FMT_RGB48BE     \
> +        || (x)==PIX_FMT_RGB48LE     \
> +        || (x)==PIX_FMT_RGB32       \

you can add these with fewer changed lines


>          || (x)==PIX_FMT_RGB32_1     \
>          || (x)==PIX_FMT_RGB24       \
>          || (x)==PIX_FMT_RGB565      \

> Index: swscale_template.c
> ===================================================================
> --- swscale_template.c	(revision 29318)
> +++ swscale_template.c	(working copy)
> @@ -2130,6 +2130,48 @@ static inline void RENAME(monoblack2Y)(uint8_t *ds
>      }
>  }
>  
> +static inline void RENAME(rgb48ToY)(uint8_t *dst, const uint8_t *src, int width)
> +{
> +    int i;
> +    for (i = 0; i < width; i++) {
> +        int r = src[i*6+0];
> +        int g = src[i*6+2];
> +        int b = src[i*6+4];
> +
> +        dst[i] = ((RY*r + GY*g + BY*b + (33<<(RGB2YUV_SHIFT-1))) >> RGB2YUV_SHIFT);
> +    }
> +}

these functions will be duplicated as swscale_template is included multiple
times


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

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/20090522/bc329115/attachment.pgp>



More information about the ffmpeg-devel mailing list