[FFmpeg-devel] [RFC] RGB32 / BGR32 ethernal bug

[Michael Niedermayer]

On Sun, Jan 31, 2010 at 01:48:23AM +0100, Stefano Sabatini wrote:
> On date Tuesday 2010-01-26 09:17:12 +0100, Michael Niedermayer encoded:
> > On Tue, Jan 26, 2010 at 01:06:18AM +0100, Stefano Sabatini wrote:
> > > I, currently we have:
> > > isRGB(RGB32   = LE:BGRA) == 1
> > > isRGB(RGB32_1 = LE:ABGR) == 1
> > > 
> > > and similarly for isBGR(), this is the cause of an ethernal confusion
> > > and frustration for who dares to takle the understanding of rgba
> > > conversion code, my proposal is to change how the RGB stuff is dealt
> > > and to use in the isRGB()/isBGRA() definitions something like:
> > > 
> > > #define isRGB(x)        (           \
> > >            (x)==PIX_FMT_RGBA        \
> > >         || (x)==PIX_FMT_ARGB        \
> > >         ...
> > > 
> > > #define isBGR(x)        (           \
> > >            (x)==PIX_FMT_BGRA        \
> > >         || (x)==PIX_FMT_ABGR        \
> > >         ...
> > > 
> > > which is also what the random sane person would expect, and to do the
> > > necessary changes in the swscale code.
> > 
> > ive cleaned up the macros somewhat, which should also make their meaning
> > clearer and ive added 2 so you can add your byte based shuffling code.
> > 
> > I think it goes without saying that adding new macros and using them
> > is fine but giving a new and completely fliped meaning to existing macros
> > is not fine. At least if people expect me to maintain this in the future.
> > also your mail sounds only consistent because you skiped telling us what
> > you do with the majority of rgb formats that arent byte based. like RGB15
> > they dont fit in your sheme at all
> > 
> > 
> > >  rgb2rgb.c |   25 +++++++++++++++++++++++++
> > >  rgb2rgb.h |    7 +++++++
> > >  swscale.c |   29 ++++++++++++++++++++++++++---
> > >  3 files changed, 58 insertions(+), 3 deletions(-)
> > > 71929befe054ce487ac7ca8be14acd67d7e496fb  fix-rgba-scaling.patch
> > > Index: ffmpeg/libswscale/rgb2rgb.c
> > > ===================================================================
> > > --- ffmpeg.orig/libswscale/rgb2rgb.c	2010-01-20 23:06:57.000000000 +0100
> > > +++ ffmpeg/libswscale/rgb2rgb.c	2010-01-20 23:07:01.000000000 +0100
> > > @@ -442,3 +442,28 @@
> > >          dst[i] = ((b<<1)&0x07) | ((g&0x07)<<3) | ((r&0x03)<<6);
> > >      }
> > >  }
> > > +
> > > +void rgb32torgb32_0123(const uint8_t *src, uint8_t *dst, long src_size)
> > > +{
> > > +    memcpy(dst, src, src_size);
> > > +}
> > > +
> > > +#define RGB32_TO_RGB32_TRANSFORM_CONV(a, b, c, d)                       \
> > > +void rgb32torgb32_##a##b##c##d(const uint8_t *src, uint8_t *dst, long src_size) \
> > > +{                                                                       \
> > > +    long i;                                                             \
> > > +                                                                        \
> > > +    for (i = 0; i < src_size; i+=4) {                                   \
> > > +        dst[i + 0] = src[i + (a)];                                      \
> > > +        dst[i + 1] = src[i + (b)];                                      \
> > > +        dst[i + 2] = src[i + (c)];                                      \
> > > +        dst[i + 3] = src[i + (d)];                                      \
> > > +    }                                                                   \
> > > +}
> > > +
> > > +RGB32_TO_RGB32_TRANSFORM_CONV(0, 3, 2, 1);
> > > +RGB32_TO_RGB32_TRANSFORM_CONV(1, 2, 3, 0);
> > > +RGB32_TO_RGB32_TRANSFORM_CONV(2, 1, 0, 3);
> > > +RGB32_TO_RGB32_TRANSFORM_CONV(3, 0, 1, 2);
> > > +RGB32_TO_RGB32_TRANSFORM_CONV(3, 2, 1, 0);
> > 
> > you write byte shuffling code, id suggest to call it accordingly
> > theres nothing rgb specific on this, it could be usefull to do packed yuv
> > shuffling as well.
> > 
> > like maybe:
> > byte_shuffle_0123_to_0321
> > but before you convert your code to this, what is the speed gain from
> > a/b/c/d being constants instead of just passed into the function?
> > there should be enough registers for this in theory
> > also
> > dst+=4; src+=4
> > for(dst<dstend
> > would reduce the register need
> 
> Patch+diff of the regression test attached, maybe rgb2rgb.{h,c} is not
> the right place for the shuffle functions and you can suggest a better
> place.

it can be moved later


> 
> I have yet to try/benchmark with a generic shuffle_byte_4() function.

then ill wait with a review

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20100131/4c5669e5/attachment.pgp>