[FFmpeg-devel] [PATCH] libswscale/swscale_unscaled: fix DITHER_COPY macro
Michael Niedermayer
michael at niedermayer.cc
Wed Sep 6 03:07:30 EEST 2017
On Wed, Sep 06, 2017 at 01:25:45AM +0200, Mateusz wrote:
> W dniu 2017-09-05 o 23:37, Michael Niedermayer pisze:
> > On Tue, Sep 05, 2017 at 04:42:06PM +0200, Mateusz wrote:
> >> W dniu 2017-09-05 o 15:40, Michael Niedermayer pisze:
> >>> On Mon, Sep 04, 2017 at 09:33:34AM +0200, Mateusz wrote:
> >>>> If ffmpeg reduces bit-depth to 8-bit or more, it uses DITHER_COPY macro.
> >>>> The problem is DITHER_COPY macro make images darker (on all planes).
> >>>>
> >>>> In x265 project there is issue #365 which is caused by this DITHER_COPY bug.
> >>>> I think it is time to fix -- there are more and more high bit-depth sources.
> >>>>
> >>>> Please review.
> >>>>
> >>>> libswscale/swscale_unscaled.c | 44 +++++++++++++------------------------------
> >>>> 1 file changed, 13 insertions(+), 31 deletions(-)
> >>>
> >>> please provide a git compatible patch with with commit message as produced
> >>> by git format-patch or git send-email
> >>>
> >>> this mail is not accepted as is by git
> >>> Applying: libswscale/swscale_unscaled: fix DITHER_COPY macro
> >>> error: corrupt patch at line 6
> >>> error: could not build fake ancestor
> >>> Patch failed at 0001 libswscale/swscale_unscaled: fix DITHER_COPY macro
> >>>
> >>> [...]
> >>
> >> I've attached the result of git format-patch command.
> >>
> >> Sorry for 1 private e-mail (I clicked wrong button).
> >>
> >> Mateusz
> >
> >> swscale_unscaled.c | 44 +++++++++++++-------------------------------
> >> 1 file changed, 13 insertions(+), 31 deletions(-)
> >> 9973b13b3f74319abe9c97302ee87b2b3468b3b6 0001-fix-DITHER_COPY-macro-to-avoid-make-images-darker.patch
> >> From 9b96d612fef46ccec7e148cd7f8e8666b4e7a4cd Mon Sep 17 00:00:00 2001
> >> From: Mateusz <mateuszb at poczta.onet.pl>
> >> Date: Tue, 5 Sep 2017 16:09:28 +0200
> >> Subject: [PATCH] fix DITHER_COPY macro to avoid make images darker
> >>
> >> ---
> >> libswscale/swscale_unscaled.c | 44 +++++++++++++------------------------------
> >> 1 file changed, 13 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c
> >> index ef36aec..3b7a5a9 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)
> >> @@ -1502,22 +1484,22 @@ 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, 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(tmp - (tmp>>dst_depth));\
> >> + tmp = (bswap(src[j+1]) + dither[1])>>shift; dst[j+1] = dbswap(tmp - (tmp>>dst_depth));\
> >> + tmp = (bswap(src[j+2]) + dither[2])>>shift; dst[j+2] = dbswap(tmp - (tmp>>dst_depth));\
> >> + tmp = (bswap(src[j+3]) + dither[3])>>shift; dst[j+3] = dbswap(tmp - (tmp>>dst_depth));\
> >> + tmp = (bswap(src[j+4]) + dither[4])>>shift; dst[j+4] = dbswap(tmp - (tmp>>dst_depth));\
> >> + tmp = (bswap(src[j+5]) + dither[5])>>shift; dst[j+5] = dbswap(tmp - (tmp>>dst_depth));\
> >> + tmp = (bswap(src[j+6]) + dither[6])>>shift; dst[j+6] = dbswap(tmp - (tmp>>dst_depth));\
> >> + tmp = (bswap(src[j+7]) + dither[7])>>shift; dst[j+7] = dbswap(tmp - (tmp>>dst_depth));\
> >
> > This does not look correct
> >
> > flat black should be mapped to flat black (ok)
> > flat white should be mapped to flat white (ok)
> > black+1 should not be mapped to flat black but to a dithered pattern mixing black with the next brighter color (ok)
> > white-1 should not be mapped to flat white but to a dithered pattern mixing white with the next darker color (not ok)
> >
> > example if you take 9 bit to 7 bit
> > 511 + {0..3} = {511..514}
> > {511..514} >> 2 = {127,128}
> > {127,128} - ({127,128} >> 7) = 127
> >
> > 510 + {0..3} = {510..513}
> > {510..513} >> 2 = {127,128}
> > {127,128} - ({127,128} >> 7) = 127
> >
> > Thus multiple of the brigher colors all get mapped to the same output,
> > that way the brightest shades become indistingishable and this is not
> > correct.
> >
> > above is based on code review not testing so i might have missed
> > something
> >
> > [...]
>
> Yes, you are right.
>
> There is one more rule, that is important and (partially) in contradiction to white-1 to not flat white rule:
> if you convert 7-bit source to 9-bit and next resulting 9-bit to 7-bit, it should be the same picture as original.
this would be desirable
> if we convert 7 bit white (or max value at plane) == 127, we have 127 << 2 == 508
> now 508 goes always to 127
This would only be the case if 508 was the white point and not 511
otherwise
If you convert 7bit to 9bit, 127 is converted to 511, this would be a
requirement to maintain white.
if you use fewer bits it becomes more obvious
2 bits to 4 bits
3 -> 15 not 3 -> 12
whereever the white and black points are in the input and output
colorspace they must get converted correctly.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170906/03da837d/attachment.sig>
More information about the ffmpeg-devel
mailing list