[FFmpeg-devel] [PATCH] libswscale/swscale_unscaled: fix DITHER_COPY macro

Mateusz Brzostek mateusz at msystem.waw.pl
Sat Mar 18 22:28:10 EET 2017


W dniu 2017-03-16 o 19:17, Michael Niedermayer pisze:
> On Wed, Mar 15, 2017 at 10:52:29PM +0100, Mateusz Brzostek wrote:
>> Hello!
>>
>> There are 3 problems with DITHER_COPY macro in libswscale/swscale_unscaled.c:
>> 1) there is overflow in dithering from 12-bit to 10-bit (output value > 1023);
>> 2) for limit range the lower limit is not respected, for example from 10-bit to 8-bit value 64 is converted to 15;
>> 3) for many iteration of downscale/upscale of the same image the 200th iteration is significantly darker.
>>
>> The first bug is due to wrong dithers table (now it is OK only for 8-bit destination), fix is:
>> -        const uint8_t *dither= dithers[src_depth-9][i&7];\
>> +        const uint8_t *dither= dithers[src_depth-dst_depth-1][i&7];\
>>
>> For bugs 2) and 3) it is needed formula that do not make images darker (in attachment). So please review.
> does your code maintain white and black levels ?
> with 4 bits white is 15, with 7 bits white is 127 for example
> white should stay white
> black should stay black
> in both directions
>
> [...]
After speed tests I've prepared faster version of this patch (do exactly the same but faster).

ffmpeg0 - clean 64-bit gcc 6.3 build for Windows from snapshot 2017-03-18,
ffmpeg1 - the same + old patch,
ffmpeg2 - the same + new patch.

Average speed was (from 10 laps):
ffmpeg0 -- 2.625
ffmpeg1 -- 2.540
ffmpeg2 -- 2.628

New patch is faster than old and is not slower than current (buggy) code.

My speed test looks like this (copy from 324 frames movie 2400x1350, yuv444p12):
f:\speed\ff2>for /L %n in (1 1 10) do (for /L %i in (0 1 2) do (ffmpeg%i -y -stats -i ../original_vc3.mxf -v error -strict -1 -p
ix_fmt yuv444p10 -f yuv4mpegpipe NUL ) )

f:\speed\ff2>(for /L %i in (0 1 2) do (ffmpeg%i -y -stats -i ../original_vc3.mxf -v error -strict -1 -pix_fmt yuv444p10 -f yuv4m
pegpipe NUL ) )

f:\speed\ff2>(ffmpeg0 -y -stats -i ../original_vc3.mxf -v error -strict -1 -pix_fmt yuv444p10 -f yuv4mpegpipe NUL )
frame=  324 fps= 63 q=-0.0 Lsize= 6150939kB time=00:00:13.50 bitrate=3732481.2kbits/s speed=2.64x

f:\speed\ff2>(ffmpeg1 -y -stats -i ../original_vc3.mxf -v error -strict -1 -pix_fmt yuv444p10 -f yuv4mpegpipe NUL )
frame=  324 fps= 61 q=-0.0 Lsize= 6150939kB time=00:00:13.50 bitrate=3732481.2kbits/s speed=2.54x

f:\speed\ff2>(ffmpeg2 -y -stats -i ../original_vc3.mxf -v error -strict -1 -pix_fmt yuv444p10 -f yuv4mpegpipe NUL )
frame=  324 fps= 63 q=-0.0 Lsize= 6150939kB time=00:00:13.50 bitrate=3732481.2kbits/s speed=2.62x

New patch inline (+ in attachment for easy apply):
 libswscale/swscale_unscaled.c | 41 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c
index ba3d688..a1d0a8d 100644
--- a/libswscale/swscale_unscaled.c
+++ b/libswscale/swscale_unscaled.c
@@ -110,24 +110,6 @@ DECLARE_ALIGNED(8, static const uint8_t, dithers)[8][8][8]={
   { 112, 16,104,  8,118, 22,110, 14,},
 }};
 
-static const uint16_t dither_scale[15][16]={
-{    2,    3,    3,    5,    5,    5,    5,    5,    5,    5,    5,    5,    5,    5,    5,    5,},
-{    2,    3,    7,    7,   13,   13,   25,   25,   25,   25,   25,   25,   25,   25,   25,   25,},
-{    3,    3,    4,   15,   15,   29,   57,   57,   57,  113,  113,  113,  113,  113,  113,  113,},
-{    3,    4,    4,    5,   31,   31,   61,  121,  241,  241,  241,  241,  481,  481,  481,  481,},
-{    3,    4,    5,    5,    6,   63,   63,  125,  249,  497,  993,  993,  993,  993,  993, 1985,},
-{    3,    5,    6,    6,    6,    7,  127,  127,  253,  505, 1009, 2017, 4033, 4033, 4033, 4033,},
-{    3,    5,    6,    7,    7,    7,    8,  255,  255,  509, 1017, 2033, 4065, 8129,16257,16257,},
-{    3,    5,    6,    8,    8,    8,    8,    9,  511,  511, 1021, 2041, 4081, 8161,16321,32641,},
-{    3,    5,    7,    8,    9,    9,    9,    9,   10, 1023, 1023, 2045, 4089, 8177,16353,32705,},
-{    3,    5,    7,    8,   10,   10,   10,   10,   10,   11, 2047, 2047, 4093, 8185,16369,32737,},
-{    3,    5,    7,    8,   10,   11,   11,   11,   11,   11,   12, 4095, 4095, 8189,16377,32753,},
-{    3,    5,    7,    9,   10,   12,   12,   12,   12,   12,   12,   13, 8191, 8191,16381,32761,},
-{    3,    5,    7,    9,   10,   12,   13,   13,   13,   13,   13,   13,   14,16383,16383,32765,},
-{    3,    5,    7,    9,   10,   12,   14,   14,   14,   14,   14,   14,   14,   15,32767,32767,},
-{    3,    5,    7,    9,   11,   12,   14,   15,   15,   15,   15,   15,   15,   15,   16,65535,},
-};
-
 
 static void fillPlane(uint8_t *plane, int stride, int width, int height, int y,
                       uint8_t val)
@@ -1485,22 +1467,21 @@ static int packedCopyWrapper(SwsContext *c, const uint8_t *src[],
 }
 
 #define DITHER_COPY(dst, dstStride, src, srcStride, bswap, dbswap)\
-    uint16_t scale= dither_scale[dst_depth-1][src_depth-1];\
-    int shift= src_depth-dst_depth + dither_scale[src_depth-2][dst_depth-1];\
+    unsigned shift= src_depth-dst_depth, dst_max= (1<<dst_depth)-1, tmp;\
     for (i = 0; i < height; i++) {\
-        const uint8_t *dither= dithers[src_depth-9][i&7];\
+        const uint8_t *dither= dithers[shift-1][i&7];\
         for (j = 0; j < length-7; j+=8){\
-            dst[j+0] = dbswap((bswap(src[j+0]) + dither[0])*scale>>shift);\
-            dst[j+1] = dbswap((bswap(src[j+1]) + dither[1])*scale>>shift);\
-            dst[j+2] = dbswap((bswap(src[j+2]) + dither[2])*scale>>shift);\
-            dst[j+3] = dbswap((bswap(src[j+3]) + dither[3])*scale>>shift);\
-            dst[j+4] = dbswap((bswap(src[j+4]) + dither[4])*scale>>shift);\
-            dst[j+5] = dbswap((bswap(src[j+5]) + dither[5])*scale>>shift);\
-            dst[j+6] = dbswap((bswap(src[j+6]) + dither[6])*scale>>shift);\
-            dst[j+7] = dbswap((bswap(src[j+7]) + dither[7])*scale>>shift);\
+            tmp = (bswap(src[j+0]) + dither[0])>>shift; dst[j+0] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+1]) + dither[1])>>shift; dst[j+1] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+2]) + dither[2])>>shift; dst[j+2] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+3]) + dither[3])>>shift; dst[j+3] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+4]) + dither[4])>>shift; dst[j+4] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+5]) + dither[5])>>shift; dst[j+5] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+6]) + dither[6])>>shift; dst[j+6] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+7]) + dither[7])>>shift; dst[j+7] = dbswap(FFMIN(tmp, dst_max));\
         }\
         for (; j < length; j++)\
-            dst[j] = dbswap((bswap(src[j]) + dither[j&7])*scale>>shift);\
+            tmp = (bswap(src[j]) + dither[j&7])>>shift; dst[j] = dbswap(FFMIN(tmp, dst_max));\
         dst += dstStride;\
         src += srcStride;\
     }
-------------- next part --------------
 libswscale/swscale_unscaled.c | 41 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c
index ba3d688..a1d0a8d 100644
--- a/libswscale/swscale_unscaled.c
+++ b/libswscale/swscale_unscaled.c
@@ -110,24 +110,6 @@ DECLARE_ALIGNED(8, static const uint8_t, dithers)[8][8][8]={
   { 112, 16,104,  8,118, 22,110, 14,},
 }};
 
-static const uint16_t dither_scale[15][16]={
-{    2,    3,    3,    5,    5,    5,    5,    5,    5,    5,    5,    5,    5,    5,    5,    5,},
-{    2,    3,    7,    7,   13,   13,   25,   25,   25,   25,   25,   25,   25,   25,   25,   25,},
-{    3,    3,    4,   15,   15,   29,   57,   57,   57,  113,  113,  113,  113,  113,  113,  113,},
-{    3,    4,    4,    5,   31,   31,   61,  121,  241,  241,  241,  241,  481,  481,  481,  481,},
-{    3,    4,    5,    5,    6,   63,   63,  125,  249,  497,  993,  993,  993,  993,  993, 1985,},
-{    3,    5,    6,    6,    6,    7,  127,  127,  253,  505, 1009, 2017, 4033, 4033, 4033, 4033,},
-{    3,    5,    6,    7,    7,    7,    8,  255,  255,  509, 1017, 2033, 4065, 8129,16257,16257,},
-{    3,    5,    6,    8,    8,    8,    8,    9,  511,  511, 1021, 2041, 4081, 8161,16321,32641,},
-{    3,    5,    7,    8,    9,    9,    9,    9,   10, 1023, 1023, 2045, 4089, 8177,16353,32705,},
-{    3,    5,    7,    8,   10,   10,   10,   10,   10,   11, 2047, 2047, 4093, 8185,16369,32737,},
-{    3,    5,    7,    8,   10,   11,   11,   11,   11,   11,   12, 4095, 4095, 8189,16377,32753,},
-{    3,    5,    7,    9,   10,   12,   12,   12,   12,   12,   12,   13, 8191, 8191,16381,32761,},
-{    3,    5,    7,    9,   10,   12,   13,   13,   13,   13,   13,   13,   14,16383,16383,32765,},
-{    3,    5,    7,    9,   10,   12,   14,   14,   14,   14,   14,   14,   14,   15,32767,32767,},
-{    3,    5,    7,    9,   11,   12,   14,   15,   15,   15,   15,   15,   15,   15,   16,65535,},
-};
-
 
 static void fillPlane(uint8_t *plane, int stride, int width, int height, int y,
                       uint8_t val)
@@ -1485,22 +1467,21 @@ static int packedCopyWrapper(SwsContext *c, const uint8_t *src[],
 }
 
 #define DITHER_COPY(dst, dstStride, src, srcStride, bswap, dbswap)\
-    uint16_t scale= dither_scale[dst_depth-1][src_depth-1];\
-    int shift= src_depth-dst_depth + dither_scale[src_depth-2][dst_depth-1];\
+    unsigned shift= src_depth-dst_depth, dst_max= (1<<dst_depth)-1, tmp;\
     for (i = 0; i < height; i++) {\
-        const uint8_t *dither= dithers[src_depth-9][i&7];\
+        const uint8_t *dither= dithers[shift-1][i&7];\
         for (j = 0; j < length-7; j+=8){\
-            dst[j+0] = dbswap((bswap(src[j+0]) + dither[0])*scale>>shift);\
-            dst[j+1] = dbswap((bswap(src[j+1]) + dither[1])*scale>>shift);\
-            dst[j+2] = dbswap((bswap(src[j+2]) + dither[2])*scale>>shift);\
-            dst[j+3] = dbswap((bswap(src[j+3]) + dither[3])*scale>>shift);\
-            dst[j+4] = dbswap((bswap(src[j+4]) + dither[4])*scale>>shift);\
-            dst[j+5] = dbswap((bswap(src[j+5]) + dither[5])*scale>>shift);\
-            dst[j+6] = dbswap((bswap(src[j+6]) + dither[6])*scale>>shift);\
-            dst[j+7] = dbswap((bswap(src[j+7]) + dither[7])*scale>>shift);\
+            tmp = (bswap(src[j+0]) + dither[0])>>shift; dst[j+0] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+1]) + dither[1])>>shift; dst[j+1] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+2]) + dither[2])>>shift; dst[j+2] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+3]) + dither[3])>>shift; dst[j+3] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+4]) + dither[4])>>shift; dst[j+4] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+5]) + dither[5])>>shift; dst[j+5] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+6]) + dither[6])>>shift; dst[j+6] = dbswap(FFMIN(tmp, dst_max));\
+            tmp = (bswap(src[j+7]) + dither[7])>>shift; dst[j+7] = dbswap(FFMIN(tmp, dst_max));\
         }\
         for (; j < length; j++)\
-            dst[j] = dbswap((bswap(src[j]) + dither[j&7])*scale>>shift);\
+            tmp = (bswap(src[j]) + dither[j&7])>>shift; dst[j] = dbswap(FFMIN(tmp, dst_max));\
         dst += dstStride;\
         src += srcStride;\
     }


More information about the ffmpeg-devel mailing list