[Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.

[Shiyou Yin]

Will this patch be merged?

>-----Original Message-----
>From: pixman-bounces at lists.freedesktop.org [mailto:pixman-bounces at lists.freedesktop.org] On Behalf
>Of Shiyou Yin
>Sent: Monday, February 10, 2020 5:50 PM
>To: 'Søren Sandmann'
>Cc: 'pixman'
>Subject: Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.
>
>That's great, the analysis is very detailed.
>Macro RB_MASK_PLUS_ONE is mainly used to saturate the 16-bits data to 0xff.
>In function UN8_rb_ADD_UN8_rb, it can’t affect the result in the end.
>
>
>>-----Original Message-----
>>From: Søren Sandmann [mailto:soren.sandmann at gmail.com]
>>Sent: Sunday, February 9, 2020 11:44 PM
>>To: Shiyou Yin
>>Cc: Matt Turner; pixman
>>Subject: Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.
>>
>>Hi,
>>
>>The reason this bug doesn't show up in the test suite is that it
>>doesn't have an effect on the output. The purpose of the
>>UN8_rb_ADD_UN8_rb macro is to add two numbers like these:
>>
>>    00aa00bb
>>    00cc00dd
>>
>>together, but to saturate aa+cc and bb+dd to 0xff instead of allowing
>>them to overflow. Let's pretend that bb and dd are both zero and focus
>>on aa+cc. If that addition does in fact overflow (ie., if aa+cc >
>>0xff), the result looks like this:
>>
>>    01??0000
>>
>>The macro first detects the overflow shifting this value by 8 and
>>masking to 0x00ff00ff:
>>
>>    00010000
>>
>>Then it subtracts this from the RB_MASK_PLUS_ONE value:
>>
>>    with bug:       10000000 - 00010000 = 0fff0000
>>    with bug fix:   01000000 - 00010000 = 00ff0000
>>
>>The macro then ors this value with the original sum:
>>
>>    with bug:        0fff0000 | 01??0000 = 0fff0000
>>    with bug fix:    00ff0000 | 01??0000 = 01ff0000
>>
>>But then it masks the result with 00ff00ff, and so both with and
>>without the bug, the final result is the same:
>>
>>    00ff0000
>>
>>which is why the bug doesn't affect the calculation.
>>
>>
>>Søren
>>
>>
>>On Sun, Feb 9, 2020 at 4:26 AM Shiyou Yin <yinshiyou-hf at loongson.cn> wrote:
>>>
>>> >-----Original Message-----
>>> >From: Matt Turner [mailto:mattst88 at gmail.com]
>>> >Sent: Sunday, February 9, 2020 7:01 AM
>>> >To: Yin Shiyou
>>> >Cc: pixman at lists.freedesktop.org
>>> >Subject: Re: [Pixman] [PATCH] pixman-combine: Fix wrong value of RB_MASK_PLUS_ONE.
>>> >
>>> >On Mon, Feb 3, 2020 at 1:56 AM Yin Shiyou <yinshiyou-hf at loongson.cn> wrote:
>>> >>
>>> >> ---
>>> >>  pixman/pixman-combine32.h | 2 +-
>>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/pixman/pixman-combine32.h b/pixman/pixman-combine32.h
>>> >> index cdd56a6..59bb247 100644
>>> >> --- a/pixman/pixman-combine32.h
>>> >> +++ b/pixman/pixman-combine32.h
>>> >> @@ -12,7 +12,7 @@
>>> >>  #define RB_MASK 0xff00ff
>>> >>  #define AG_MASK 0xff00ff00
>>> >>  #define RB_ONE_HALF 0x800080
>>> >> -#define RB_MASK_PLUS_ONE 0x10000100
>>> >> +#define RB_MASK_PLUS_ONE 0x1000100
>>> >
>>> >
>>> >Thanks. The patch looks correct, but obviously nothing in the test
>>> >suite is failing. How did you discover this? Does this patch fix
>>> >something for you?
>>>
>>> I just found this mistake while analyzing macro 'UN8_rb_ADD_UN8_rb'
>>> and functions called this macro. I can't verify this error with existing
>>> test suite, it all passed no matter Fix it or not.
>>>
>>> _______________________________________________
>>> Pixman mailing list
>>> Pixman at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/pixman
>
>_______________________________________________
>Pixman mailing list
>Pixman at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/pixman