[FFmpeg-devel] [RFC] Make swscale-test perform only one convertion
Michael Niedermayer
michaelni
Fri Feb 19 01:33:59 CET 2010
On Fri, Feb 19, 2010 at 12:23:36AM +0100, Stefano Sabatini wrote:
> On date Thursday 2010-02-18 09:29:19 +0100, Stefano Sabatini encoded:
> > On date Thursday 2010-02-18 04:10:44 -0200, Ramiro Polla encoded:
> > > On Thu, Feb 18, 2010 at 2:25 AM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
> > > > swscale-test is currently crashing in Windows in ./swscale-test.exe
> > > > rgb24 argb. Apparently due to heap corruption (that's what windows
> > > > says), but I couldn't find out the exact cause yet.
> > >
> > > It crashes on:
> > > rgb24 96x96 -> argb 96x 96 flags=16
> > >
> > > While trying to free dst in swscale-test.c:doTest()
> > > for (i=0; i<4; i++) {
> > > av_free(src[i]);
> > > av_free(dst[i]);
> > > av_free(out[i]);
> > > }
> > >
> > > The function rgb24tobgr32 has overwritten data past the end of its
> > > destination pointer because swscale.c:rgbToRgbWrapper() checks:
> > > if ((dstFormat == PIX_FMT_RGB32_1 || dstFormat ==
> > > PIX_FMT_BGR32_1) && !isRGBA32(srcFormat))
> > > dstPtr += ALT32_CORR;
> > > and increments dstPtr.
> > >
> > > Stefano, I see that you have added that !isRGBA32() there. Could you
> > > double check if this is correct for all cases? Shouldn't it actually
> > > be:
> > >
> > > Index: swscale.c
> > > ===================================================================
> > > --- swscale.c (revision 30631)
> > > +++ swscale.c (working copy)
> > > @@ -1502,10 +1502,10 @@
> > > } else {
> > > const uint8_t *srcPtr= src[0];
> > > uint8_t *dstPtr= dst[0];
> > > - if ((srcFormat == PIX_FMT_RGB32_1 || srcFormat ==
> > > PIX_FMT_BGR32_1) && !isRGBA32(dstFormat))
> > > + if ((srcFormat == PIX_FMT_RGB32_1 || srcFormat ==
> > > PIX_FMT_BGR32_1) && !isRGBA32(srcFormat))
> > > srcPtr += ALT32_CORR;
> > >
> > > - if ((dstFormat == PIX_FMT_RGB32_1 || dstFormat ==
> > > PIX_FMT_BGR32_1) && !isRGBA32(srcFormat))
> > > + if ((dstFormat == PIX_FMT_RGB32_1 || dstFormat ==
> > > PIX_FMT_BGR32_1) && !isRGBA32(dstFormat))
> > > dstPtr += ALT32_CORR;
> > >
> > > if (dstStride[0]*srcBpp == srcStride[0]*dstBpp && srcStride[0] > 0)
> >
> > No this is wrong.
> >
> > But I see there is a problem here, we have rgb24to32 writing over the
> > end of the slice. Maybe we need to implement some specific function
> > for dealing with that case (BTW we are also skipping to write the
> > first byte of the ARGB array).
>
> Please try the attached patch.
>
> BTW we should find some way to bypass the rgbToRgbWrapper and cache
> conv function once and for all.
>
> Regards.
> --
> FFmpeg = Fostering Fascinating Muttering Peaceful Everlasting Guru
> rgb2rgb.c | 31 +++++++++++++++++++++++++++++++
> rgb2rgb.h | 5 +++++
> swscale.c | 23 +++++++++++++++++++----
> 3 files changed, 55 insertions(+), 4 deletions(-)
> 42d7b55826f1b1a497f1f6c17f374832ee1579ec fix-rgb24-to32-conv.patch
> Index: ffmpeg/libswscale/rgb2rgb.c
> ===================================================================
> --- ffmpeg.orig/libswscale/rgb2rgb.c 2010-02-18 23:04:47.000000000 +0100
> +++ ffmpeg/libswscale/rgb2rgb.c 2010-02-19 00:16:02.000000000 +0100
> @@ -462,3 +462,34 @@
> DEFINE_SHUFFLE_BYTES(3, 0, 1, 2);
> DEFINE_SHUFFLE_BYTES(3, 2, 1, 0);
>
> +#define DEFINE_SHUFFLE_BYTES_ABCX(a, b, c) \
> +void shuffle_bytes_##a##b##c##X(const uint8_t *src, uint8_t *dst, long src_size) \
> +{ \
> + long i, j; \
> + \
> + for (i = 0, j = 0; i < src_size; i+=3, j+=4) { \
> + dst[j + 0] = src[i + a]; \
> + dst[j + 1] = src[i + b]; \
> + dst[j + 2] = src[i + c]; \
> + dst[j + 3] = 255; \
> + } \
> +}
> +
> +DEFINE_SHUFFLE_BYTES_ABCX(0, 1, 2);
> +DEFINE_SHUFFLE_BYTES_ABCX(2, 1, 0);
> +
> +#define DEFINE_SHUFFLE_BYTES_XABC(a, b, c) \
> +void shuffle_bytes_X##a##b##c(const uint8_t *src, uint8_t *dst, long src_size) \
> +{ \
> + long i, j; \
> + \
> + for (i = 0, j = 0; i < src_size; i+=3, j+=4) { \
> + dst[j + 0] = 255; \
> + dst[j + 1] = src[i + a]; \
> + dst[j + 2] = src[i + b]; \
> + dst[j + 3] = src[i + c]; \
> + } \
> +}
> +
> +DEFINE_SHUFFLE_BYTES_XABC(0, 1, 2);
> +DEFINE_SHUFFLE_BYTES_XABC(2, 1, 0);
> Index: ffmpeg/libswscale/rgb2rgb.h
> ===================================================================
> --- ffmpeg.orig/libswscale/rgb2rgb.h 2010-02-18 23:18:53.000000000 +0100
> +++ ffmpeg/libswscale/rgb2rgb.h 2010-02-18 23:34:41.000000000 +0100
> @@ -66,6 +66,11 @@
> void shuffle_bytes_3012(const uint8_t *src, uint8_t *dst, long src_size);
> void shuffle_bytes_3210(const uint8_t *src, uint8_t *dst, long src_size);
>
> +void shuffle_bytes_012X(const uint8_t *src, uint8_t *dst, long src_size);
> +void shuffle_bytes_210X(const uint8_t *src, uint8_t *dst, long src_size);
> +void shuffle_bytes_X012(const uint8_t *src, uint8_t *dst, long src_size);
> +void shuffle_bytes_X210(const uint8_t *src, uint8_t *dst, long src_size);
the letter F is better than X for 255
> +
> void palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> void palette8topacked24(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> void palette8torgb16(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> 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?
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
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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20100219/4cb2f7bf/attachment.pgp>
More information about the ffmpeg-devel
mailing list