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

Michael Niedermayer michaelni
Sat May 23 03:47:46 CEST 2009


On Fri, May 22, 2009 at 08:31:41AM +0300, Kostya wrote:
> 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

so |SWS_ACCURATE_RND to that
then | SWS_BITEXACT to it


>  
> > > 
> > > [...]
> > > > 
> > > > > @@ -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. 

hmmm, well, ok


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

if its not building different variants then t doesnt belong in the template


> That's an issue with most of
> non-YUV functions there.

sws needs some cleanup ...

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

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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/20090523/00351f6b/attachment.pgp>



More information about the ffmpeg-devel mailing list