Fix the PNCodec error on Android caused by different Skia color format.

Fix the PNCodec error on Android caused by different Skia color format.

Make sure the test case to compare SkPMColor between SkPMColor, rather than SkColor.

BUG=138933
TEST=ui_unittests --gtest_filter=PNGCodec.EncodeBGRASkBitmapDiscardTransparency

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149208

[Shouqun Liu, 2012-07-26 07:58:25]

Hi, this patch fixes a PNGCodec failed case on Android (Bug 138933), the failure
is cause by SKColorGet. 

Since on Android, the Skia color format is ABGR, which is different from
Chromium's ARGB, SKColorGetR is hard-coded to get color channel (((color) >>
16)),  so change it to SkGetPackedR32 (use SK_R32_SHIFT) and then fix the failed
case.

PNGCodec.* test pass,  thanks for review

[tony, 2012-07-26 17:38:21]

Are the other uses of SkColorGet{R,G,B} in the code incorrect?

http://codereview.chromium.org/10831031/diff/1/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

http://codereview.chromium.org/10831031/diff/1/ui/gfx/codec/png_codec.cc#newc...
ui/gfx/codec/png_codec.cc:69: pixel_out[2] = SkGetPackedB32(unmultiplied);
Since the else block does the same as this, I would restructure this code to:

if (alpha != 0 && alpha != 255) {
  pixel_in = SkUnPreMultiply::PMColorToColor(pixel_in);
pixel_out[0] = SkGetPackedR32(pixel_in);
pixel_out[1] = SkGetPackedG32(pixel_in);
pixel_out[2] = SkGetPackedB32(pixel_in);

[tony, 2012-07-26 18:30:14]

Also, seems like http://codereview.chromium.org/10823012/ is related.

[Shouqun Liu, 2012-07-27 00:31:48]

Hi tony, I think for chromium on other platforms, it's correct, but for chromium

on  android,  it's incorrect. So to make it to handle more general cases, I
think 
we  should change SkColorGet{R,G,B}  to SkGetPacked{R,G,B}32.

The patch has been updated, thanks!

On 2012/07/26 17:38:21, tony wrote:
> Are the other uses of SkColorGet{R,G,B} in the code incorrect?
> 
> http://codereview.chromium.org/10831031/diff/1/ui/gfx/codec/png_codec.cc
> File ui/gfx/codec/png_codec.cc (right):
> 
>
http://codereview.chromium.org/10831031/diff/1/ui/gfx/codec/png_codec.cc#newc...
> ui/gfx/codec/png_codec.cc:69: pixel_out[2] = SkGetPackedB32(unmultiplied);
> Since the else block does the same as this, I would restructure this code to:
> 
> if (alpha != 0 && alpha != 255) {
>   pixel_in = SkUnPreMultiply::PMColorToColor(pixel_in);
> pixel_out[0] = SkGetPackedR32(pixel_in);
> pixel_out[1] = SkGetPackedG32(pixel_in);
> pixel_out[2] = SkGetPackedB32(pixel_in);

[Shouqun Liu, 2012-07-27 00:40:03]

yes, similar color format issue on android but different solution,  this patch
fixes the PNGCodec error and guarantees that we can always get correct result.

On 2012/07/26 18:30:14, tony wrote:
> Also, seems like http://codereview.chromium.org/10823012/ is related.

[tony, 2012-07-27 16:56:58]

LGTM, but I don't know much about skia.  Adding a couple skia folks to weigh in.

[Yaron, 2012-07-27 17:04:19]

Removing myself as reviewer. I don't have familiarity with this stuff; it's much
better in skia folks' hands. The unit_test filter being updated is fine assuming
the test now passes

[reed1, 2012-07-27 17:45:13]

Why the change to the pixel unpacking?

The output of the unpremultiply call is definitely SkColor, which should be
unpacked with SkColorGetR, ...

[Shouqun Liu, 2012-07-28 02:09:12]

Thanks, reed, the reason I change SkColor to SkGetPackedR32 is  that on Android,
the color format is ABGR:
 #define SK_R32_SHIFT    0
 #define SK_G32_SHIFT    8
 #define SK_B32_SHIFT    16
 #define SK_A32_SHIFT    24

and the SkColorGet{R,G,B} assume the color format is ARGB:  
 #define SkColorGetA(color)      (((color) >> 24) & 0xFF)
 #define SkColorGetR(color)      (((color) >> 16) & 0xFF)
 #define SkColorGetG(color)      (((color) >>  8) & 0xFF)

I think here is a mismatch, SkGetPackedR32 uses SK_R32_SHIFT and can get this
case passed.  

On 2012/07/27 17:45:13, reed1 wrote:
> Why the change to the pixel unpacking?
> 
> The output of the unpremultiply call is definitely SkColor, which should be
> unpacked with SkColorGetR, ...

[reed1, 2012-07-30 12:52:56]

I still cannot see how this is correct, esp. given that ConvertSkiaToRGBA, which
seems to be logically the same as ConvertSkiaToRGB except that it stores the
alpha channel, is coded the same as this function (before your CL).

If the input is really in SkPMColor order (i.e. respecting the SHIFT macros in
SkColorPriv.h) then clearly the 'else' case is correct, since it unpacks the
color using SkGetPackedR32 etc.

If we need to unpremultiply it, we convert the pixel_in value from SkPMColor
order to SkColor order. Therefore we must unpack it using SkColor macros, as we
are not long packed respecting the SHIFT values.

How does this test confirm a correct answer? Is it comparing against a stored
PNG image?

[Shouqun Liu, 2012-07-30 14:08:12]

Thanks for your explanation,  the test EncodeBGRASkBitmapDiscardTransparency
works as follow:
(1), MakeTestSkBitmap to generate a SkBitmap (original_bitmap),
(2), PNGCodec::EncodeBGRASkBitmap to encode the SkBitmap to PNG and discard the
alpha channel,
(3), PNGCodec::Decode decode the PNG to SkBitmap (decoded_bitmap)
(4), For original_bitmap and decoded_bitmap, compare them pixel by pixel, each
pixel should be the same. But in android, the R and B channels are reversed.

It works for other platforms, for android, it was the correct with the same ARGB
configuration as other chromium platforms. 
with this commit: http://codereview.chromium.org/10806077, which changes the
SK_{R,G,B}32_SHIFT  to ABGR config, and then this case failed with R and B
channels reversed. So I made this CL to read channels according to SHIFTs.  

I'm not very familiar with the Skia, please correct me it I was wrong:)  Thanks!

On 2012/07/30 12:52:56, reed1 wrote:
> I still cannot see how this is correct, esp. given that ConvertSkiaToRGBA,
which
> seems to be logically the same as ConvertSkiaToRGB except that it stores the
> alpha channel, is coded the same as this function (before your CL).
> 
> If the input is really in SkPMColor order (i.e. respecting the SHIFT macros in
> SkColorPriv.h) then clearly the 'else' case is correct, since it unpacks the
> color using SkGetPackedR32 etc.
> 
> If we need to unpremultiply it, we convert the pixel_in value from SkPMColor
> order to SkColor order. Therefore we must unpack it using SkColor macros, as
we
> are not long packed respecting the SHIFT values.
> 
> How does this test confirm a correct answer? Is it comparing against a stored
> PNG image?

[reed1, 2012-07-30 20:02:50]

I think the real bug is in the test, not in the codec.

This is the code for the inner-loop of the test.

      uint32_t original_pixel = original_bitmap.getAddr32(0, y)[x];
      uint32_t unpremultiplied =
          SkUnPreMultiply::PMColorToColor(original_pixel);
      uint32_t decoded_pixel = decoded_bitmap.getAddr32(0, y)[x];
      EXPECT_TRUE(NonAlphaColorsClose(unpremultiplied, decoded_pixel))
          << "Original_pixel: ("
          << SkColorGetR(unpremultiplied) << ", "
          << SkColorGetG(unpremultiplied) << ", "
          << SkColorGetB(unpremultiplied) << "), "
          << "Decoded pixel: ("
          << SkColorGetR(decoded_pixel) << ", "
          << SkColorGetG(decoded_pixel) << ", "
          << SkColorGetB(decoded_pixel) << ")";

There are 3 pixels here:

1 original_pixel is a SkPMColor, since it is coming out of an SkBitmap.
2. unpremultiplied is a SkColor, since it is coming out of a call to
UnPreMultiply.
3. decoded_pixel is a SkPMColor, since it is coming out of an SkBitmap

The code should not compare unpremultiplied with either of the other two, since
it is packed differently (on machines where SkPMColor != SkColor, like android).

[Shouqun Liu, 2012-07-31 06:59:35]

yes, agree with you that the case is error, update the patch to compare
unpremultiplied original pixel with unpremultiplied decoded pixel.  Many thanks!

On 2012/07/30 20:02:50, reed1 wrote:
> I think the real bug is in the test, not in the codec.
> 
> This is the code for the inner-loop of the test.
> 
>       uint32_t original_pixel = original_bitmap.getAddr32(0, y)[x];
>       uint32_t unpremultiplied =
>           SkUnPreMultiply::PMColorToColor(original_pixel);
>       uint32_t decoded_pixel = decoded_bitmap.getAddr32(0, y)[x];
>       EXPECT_TRUE(NonAlphaColorsClose(unpremultiplied, decoded_pixel))
>           << "Original_pixel: ("
>           << SkColorGetR(unpremultiplied) << ", "
>           << SkColorGetG(unpremultiplied) << ", "
>           << SkColorGetB(unpremultiplied) << "), "
>           << "Decoded pixel: ("
>           << SkColorGetR(decoded_pixel) << ", "
>           << SkColorGetG(decoded_pixel) << ", "
>           << SkColorGetB(decoded_pixel) << ")";
> 
> There are 3 pixels here:
> 
> 1 original_pixel is a SkPMColor, since it is coming out of an SkBitmap.
> 2. unpremultiplied is a SkColor, since it is coming out of a call to
> UnPreMultiply.
> 3. decoded_pixel is a SkPMColor, since it is coming out of an SkBitmap
> 
> The code should not compare unpremultiplied with either of the other two,
since
> it is packed differently (on machines where SkPMColor != SkColor, like
android).

[reed1, 2012-07-31 13:09:49]

lgtm

[Shouqun Liu, 2012-07-31 14:07:26]

On 2012/07/31 13:09:49, reed1 wrote:
> lgtm

thanks!

[commit-bot: I haz the power, 2012-07-31 14:08:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10831031/11004

[commit-bot: I haz the power, 2012-07-31 14:08:18]

Presubmit check for 10831031-11004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/gfx

Presubmit checks took 1.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Shouqun Liu, 2012-07-31 15:22:29]

add asvitkine@ as the ui/gfx OWNER, thank!

[sky, 2012-07-31 15:32:29]

LGTM

[commit-bot: I haz the power, 2012-07-31 15:46:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10831031/11004

[commit-bot: I haz the power, 2012-07-31 17:02:18]

Change committed as 149208