[FFmpeg-devel] [RFC] Make swscale-test perform only one convertion

[Stefano Sabatini]

On date Friday 2010-02-19 01:33:59 +0100, Michael Niedermayer encoded:
> On Fri, Feb 19, 2010 at 12:23:36AM +0100, Stefano Sabatini wrote:
[...]
> > Index: ffmpeg/libswscale/swscale.c
> > ===================================================================
> > --- ffmpeg.orig/libswscale/swscale.c	2010-02-18 23:04:51.000000000 +0100
> > +++ ffmpeg/libswscale/swscale.c	2010-02-18 23:55:23.000000000 +0100
> > @@ -1430,6 +1430,11 @@
> >          || (x) == PIX_FMT_ABGR   \
> >          )
> >  
> > +#define isRGB24(x) (             \
> > +           (x) == PIX_FMT_RGB24  \
> > +        || (x) == PIX_FMT_BGR24  \
> > +        )
> > +
> >  /* {RGB,BGR}{15,16,24,32,32_1} -> {RGB,BGR}{15,16,24,32} */
> >  static int rgbToRgbWrapper(SwsContext *c, const uint8_t* src[], int srcStride[], int srcSliceY,
> >                             int srcSliceH, uint8_t* dst[], int dstStride[])
> > @@ -1458,6 +1463,16 @@
> >          else if (CONV_IS(BGRA, ABGR)
> >                || CONV_IS(RGBA, ARGB)) conv = shuffle_bytes_3012;
> >      } else
> > +        if (isRGB24(srcFormat) && isRGBA32(dstFormat)) {
> > +        if      (CONV_IS(RGB24, RGBA)
> > +              || CONV_IS(BGR24, BGRA)) conv = shuffle_bytes_012X;
> > +        else if (CONV_IS(RGB24, ARGB)
> > +              || CONV_IS(BGR24, ABGR)) conv = shuffle_bytes_X012;
> > +        else if (CONV_IS(RGB24, ABGR)
> > +              || CONV_IS(BGR24, ARGB)) conv = shuffle_bytes_X210;
> > +        else if (CONV_IS(RGB24, BGRA)
> > +              || CONV_IS(BGR24, RGBA)) conv = shuffle_bytes_210X;
> 
> this should be using switch/case as a jump table likely is faster
> 
> also iam against adding a converter for each format pair.
> there are too many and there are too many that have no practical
> relevance at all.
> swscale has a generic converter path, does it even support all these
> cases?

I currently have no idea, unless you're referring to
pixdesc.c:read_line()/write_line(), yes that could work.

> the generic path must be 100% working before a special unscaled converter
> is added also there must be a speed gain for such a converter and the
> converter must be usefull in practice.
> to me this and the related patches feel alot like "adding code because we can
> add code" not because of anything else

Current code is broken in some cases, some bytes are skipped and
sometimes there is a buffer overflow.

> IMHO the converters used should be optimized the rest removed.
> also if the generic path has any problem with rgb this must be fixed
> not bandaids added to handle a small subset of the cases with huge
> amounts of code.
> 
> also 15,16,24,argb,rgba,rgb48le,rgb48be (*2 for rgb/bgr) need almost 200
> converters in the style you work here it is completely unacceptable to add
> that many converteres for unscaled convertion or purely rgb from which not
> a single one has any practical use as the usefull one have alraedy been
> implemented years ago.

I agree that is better to have a more generic solution, albeit slower,
rather than require special converters for non practically useful
scalers.

Regards.
-- 
FFmpeg = Fierce and Fantastic Mystic Pitiless Elegant God