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

[Kostya]

On Fri, May 22, 2009 at 03:08:56AM +0200, Michael Niedermayer wrote:
> 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
 
it tests conversions for flags 1, 2, 4, 8, 16 and 32
 
> > 
> > [...]
> > > 
> > > > @@ -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

Indeed, but I have the impression those are ordered by decreasing depth. 

> >          || (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

Theoretically someone can adapt MMX counterpart from rgb24to{Y,UV}
Anyway, what do you suggest to avoid that? That's an issue with most of
non-YUV functions there.
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB