cc: Use SkPMColor consistently on readback.

cc: Use SkPMColor consistently on readback.

There are three pixel formats used in Chromium:
- SkColor (non-premultiplied, BGRA)
- SkPMColor (premultiplied, BGRA on desktop and RGBA on Android)
- OpenGL (on readback, premultiplied and RBGA -- in textures, same as SkPMColor)

Raw pixels inside SkBitmaps are of type SkPMColor, not SkColor.  This patch fixes
two cases that cause inconsistencies on Android:

1. SkColor was used in bitmaps in SoftwareRendererTest.  Use Skia methods
instead of raw pixels in the test so that the correct format is used automatically.

2. GLRenderer's readback also swizzled consistently to BGRA instead of considering the
Skia settings; switch that to use the platform's SkPMColor definition instead.
This is a no-op change on non-Android.

NOTRY=true
BUG=154528
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199043

[aelias_OOO_until_Jul13, 2013-05-08 06:08:31]

PTAL.

If you are curious, the nature of the "luck" that was causing the two other
pixel tests to unexpectedly pass was that the pixels were swizzled twice in the
TileDrawQuad test, and green and magenta happen to be palindromes in the
ClearRenderPass test.

[danakj, 2013-05-08 14:17:21]

Interesting! However, this means that GetFramebufferPixels() returns different
data with the software renderer than with the gl renderer?

In the gl renderer we swizzle stuff before returning it, I think we should also
fix the pixels here before we return them. Or please correct me if I'm wrong on
this.

The SoftwareRenderer::CopyCurrentRenderPassToBitmap function, I *think*, is
returning pixels that we'd expect?

[aelias_OOO_until_Jul13, 2013-05-08 15:51:13]

On 2013/05/08 14:17:21, danakj wrote:
> Interesting! However, this means that GetFramebufferPixels() returns different
> data with the software renderer than with the gl renderer?

Hmm, that seems to be true.  You're right, it should be consistent.

> 
> In the gl renderer we swizzle stuff before returning it, I think we should
also
> fix the pixels here before we return them. Or please correct me if I'm wrong
on
> this.
> 
> The SoftwareRenderer::CopyCurrentRenderPassToBitmap function, I *think*, is
> returning pixels that we'd expect?

That's correct in that SkBitmap expects PM-format pixels as well.

Looks like the platform difference comes from an #if OS(ANDROID) in
skia/config/SkUserConfig.h.  I wonder if we can make the Android setting global
to all platforms and avoid the swizzling loops entirely.  I'll try to find out
who knows about this.

[aelias_OOO_until_Jul13, 2013-05-08 17:26:27]

PTAL.  What I changed was to make the GLRenderer swizzle according to the
current Skia settings.  The invariant this preserves is that piping the output
of a readback into an SkBitmap will always do the right thing within a given
platform.  I changed the patch description accordingly.

On 2013/05/08 15:51:13, aelias wrote:
> Looks like the platform difference comes from an #if OS(ANDROID) in
> skia/config/SkUserConfig.h.  I wonder if we can make the Android setting
global
> to all platforms and avoid the swizzling loops entirely.  I'll try to find out
> who knows about this.

Never mind this, all the platforms have a good reason for their choice of pixel
format.

[danakj, 2013-05-08 17:38:43]

https://codereview.chromium.org/14883004/diff/15001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14883004/diff/15001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2197: // Swizzle OpenGL -> Skia byte order.
So, am I right in understanding this won't change any platform's behaviour here
except for android?

https://codereview.chromium.org/14883004/diff/15001/cc/output/software_render...
File cc/output/software_renderer_unittest.cc (right):

https://codereview.chromium.org/14883004/diff/15001/cc/output/software_render...
cc/output/software_renderer_unittest.cc:140: SkPMColor yellow =
SkPreMultiplyColor(SK_ColorYELLOW);
So I get that the readback is going to premultiplied, but I also find it odd
that you now have to do this premultiply thing when you SetPixels but only for a
software resource? For a GL resource, we don't do that right?

[aelias_OOO_until_Jul13, 2013-05-08 18:02:11]

On 2013/05/08 17:38:43, danakj wrote:
> cc/output/gl_renderer.cc:2197: // Swizzle OpenGL -> Skia byte order.
> So, am I right in understanding this won't change any platform's behaviour
here
> except for android?

Correct.

> So I get that the readback is going to premultiplied, but I also find it odd
> that you now have to do this premultiply thing when you SetPixels but only for
a
> software resource? For a GL resource, we don't do that right?

It happens implicitly.  Resources normally are rastered by Skia, therefore they
come in as SkPMColor automatically.  In the rare case that they are instead
loaded from files or produced from solid colors, they then might need some
special treatment.

Note also that because we can swizzle within shaders, we're also uploading
SkPMColor directly into textures.  So the question of what's the "native OpenGL
format" only comes up during readback.

[danakj, 2013-05-08 18:03:01]

Mmmh, okay thanks for the explanation. LGTM

[aelias_OOO_until_Jul13, 2013-05-08 18:33:53]

As a further cleanup, I changed SoftwareRendererTest to avoid raw pixel
manipulation entirely as the SkPreMultiplyColor conversions were getting yucky.

[danakj, 2013-05-08 18:35:17]

Oh nice, thanks!

[commit-bot: I haz the power, 2013-05-08 22:49:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/14883004/25001

[commit-bot: I haz the power, 2013-05-08 23:07:00]

Change committed as 199043