diff mbox

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

Message ID 89fdce7a-a6b9-189d-6d87-778e325c600c@poczta.onet.pl
State Superseded
Headers show

Commit Message

Mateusz Sept. 5, 2017, 2:42 p.m. UTC 
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
From 9b96d612fef46ccec7e148cd7f8e8666b4e7a4cd Mon Sep 17 00:00:00 2001
From: Mateusz <mateuszb@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(-)

Comments

Michael Niedermayer Sept. 5, 2017, 9:37 p.m. UTC | #1 
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@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

[...]
Mateusz Sept. 5, 2017, 11:25 p.m. UTC | #2 
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@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.
if we convert 7 bit white (or max value at plane) == 127, we have 127 << 2 == 508
now 508 goes always to 127

If you want 510 goes to 127 or 126, 508 will be not always converted to 127, so if you make multiple 7bit -> 9bit -> 7bit conversions, you will have darker result (or greener).

My point is: if we upscale bit-depth by simple shift (127 to 508), this patch is OK.

If we upscale bit-depth different for luma and chroma, we should downscale bit-depth different for luma and chroma.

Now I don't know what ffmpeg is doing when upscaling bit-depth -- I will check tomorrow.

Mateusz
Michael Niedermayer Sept. 6, 2017, 12:07 a.m. UTC | #3 
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@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.


[...]
diff mbox

Patch

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));\
+        }\
+        for (; j < length; j++){\
+            tmp = (bswap(src[j]) + dither[j&7])>>shift; dst[j] = dbswap(tmp - (tmp>>dst_depth));\
         }\
-        for (; j < length; j++)\
-            dst[j] = dbswap((bswap(src[j]) + dither[j&7])*scale>>shift);\
         dst += dstStride;\
         src += srcStride;\
     }