Upstream mulDiv255Ceil() from webkit skia/ImageBuffer.cpp to skia

http://code.google.com/p/skia/issues/detail?id=88
upstream mulDiv255Ceil() from WebKit to the skia repository.

[Stephen White, 2010-12-06 18:46:50]

http://codereview.appspot.com/3466042/diff/1/include/core/SkMath.h
File include/core/SkMath.h (right):

http://codereview.appspot.com/3466042/diff/1/include/core/SkMath.h#newcode222
include/core/SkMath.h:222: unsigned prod = SkMulS16(a, b) + 255;
This only worked for me with + 254, not + 255 (although I didn't have time to
investigate why).

Have you checked that this does not regress
canvas/philip/tests/2d.imageData.put.unchanged.html?

[noel, 2010-12-06 22:19:21]

The "unsigned prod = SkMulS16(a, b) + 255" produces "a * b + 255", and that
matches the WebKit code at
http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/skia/ImageBuff...
 
unsigned mulDiv255Ceil(unsigned a, unsigned b)
{
   unsigned value = a * b + 255;
   return (value + (value >> 8)) >> 8;
}

I can change skia to use 254 if you like, but that won't match your WebKit code
:)

[Stephen White, 2010-12-06 22:41:21]

LGTM.

On 2010/12/06 22:19:21, noel wrote:
> The "unsigned prod = SkMulS16(a, b) + 255" produces "a * b + 255", and that
> matches the WebKit code at
>
http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/skia/ImageBuff...

Sorry, the CG path uses 254.  I must've changed it to 255 when I fixed the Skia
path, since it made more sense to me.

> unsigned mulDiv255Ceil(unsigned a, unsigned b)
> {
>    unsigned value = a * b + 255;
>    return (value + (value >> 8)) >> 8;
> }
> 
> I can change skia to use 254 if you like, but that won't match your WebKit
code
> :)

Never mind...

[noel, 2010-12-06 23:10:46]

confusion reigns though, should the addition factor be 254?


On Tue, Dec 7, 2010 at 9:41 AM, <senorblanco@chromium.org> wrote:

> LGTM.
>
>
> On 2010/12/06 22:19:21, noel wrote:
>
>> The "unsigned prod = SkMulS16(a, b) + 255" produces "a * b + 255", and
>>
> that
>
>> matches the WebKit code at
>>
>
>
>
http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/skia/ImageBuff...
>
> Sorry, the CG path uses 254.  I must've changed it to 255 when I fixed
> the Skia path, since it made more sense to me.
>
>
>  unsigned mulDiv255Ceil(unsigned a, unsigned b)
>> {
>>    unsigned value = a * b + 255;
>>    return (value + (value >> 8)) >> 8;
>> }
>>
>
>  I can change skia to use 254 if you like, but that won't match your
>>
> WebKit code
>
>> :)
>>
>
> Never mind...
>
>
> http://codereview.appspot.com/3466042/
>

[Stephen White, 2010-12-06 23:25:43]

On 2010/12/06 23:10:46, noel wrote:
> confusion reigns though, should the addition factor be 254?

No, 255 is correct.  It's evaluating the same as (a * b + 254 / 255). But you'll
notice the truncating version (SkMulDiv255Trunc) does a +1 in order to get the
shifts to work out right, so it's essentially doing a * b + 1 + 254 = a * b +
255.

Here's what I wrote to check it:

#include <stdio.h>

int main(int argc, char** argv)
{
  for (int color = 0; color < 256; ++color) {
    for (int alpha = 0; alpha < 256; ++alpha) {
      int skia_temp = (color * alpha + 255);
      int skia_version = (skia_temp + (skia_temp >> 8)) >> 8;
      int webkit_version = (color * alpha + 254) / 255;
      if (skia_version != webkit_version) {
        printf("diff on color %d alpha %d:  skia %d, webkit %d\n", color, alpha,
skia_version, webkit_version);
      }
    }
  }
  return 0;
}

[noel1, 2010-12-07 00:02:30]

awesome, thx for checking, we go with 255.

[noel1, 2010-12-07 00:12:47]

Also, can you take ownership of
http://code.google.com/p/skia/issues/detail?id=88 and close when
you submit the change?

[noel, 2010-12-07 07:58:12]

Added test case test_muldiv255ceiling