Fix interchange of red and blue content when using rgba_4444_textures

Fix interchange of red and blue content when using rgba_4444_textures

Content is swizzled when texture format has different component order
than expected format by platform. RGBA_4444 implies to use 4444 bits
and does not affect component order. So, no need to swizzle contents.

BUG=399634

[USE s.singapati at gmail.com, 2014-08-04 08:18:03]

@src/cc/resources/platform_color.h
SameComponentOrder() return true if the given texture format has the same
component order as the expected format by platform. When both are NOT in same
order, then content is swizzled.

BestTextureFormat() on target device is BGRA8888, and SourceDataFormat is
SOURCE_FORMAT_BGRA8, based on the flag "SK_B32_SHIFT" in skia.

Practically, when GLTtexture is created, it is already in BGRA order. so, there
is no need of content swizzling when using rgba4444.

[enne (OOO), 2014-08-04 16:53:29]

Is it possible to write a pixel or unit test for this?

[USE s.singapati at gmail.com, 2014-08-06 16:19:18]

On 2014/08/04 16:53:29, enne wrote:
> Is it possible to write a pixel or unit test for this?

Hi Enne, I think, existing pixel tests CSS3DBlueBox, Canvas2DRedBox could be
used. But main problem is to have the correct test setup that, Platform/ GPU
which expects texture in BGRA order and browser with use_rgba_4444_textures
setting enabled.

Also, I tried to run GPU bots/tryserver, but they failed. Probably i need access
rights, and can not access
https://sites.google.com/a/google.com/chrome-infrastructure/golo/remote-acces...

Any suggestions on how we can proceed with this?

[enne (OOO), 2014-08-06 17:20:23]

GPU bots aren't going to help you here.  I do not think CSS3DBlueBox or
Canvas2DRedBox are what you want.

SameComponentOrder isn't actually used in many places.  It's used in
PrioritizedResource which is deprecated (so is not a good candidate here).  It's
used in TextureLayerImpl (which tries to byte swizzle which looks incorrect in
code).  It's used in PictureDrawQuad (where you can set the texture format
yourself).  It's also used in ManagedTileState/PictureLayerImpl (but this might
be hard to mock).

If it were me, I'd probably attempt try to write a cc pixel test (see
renderer_pixeltest.cc) using a PictureDrawQuad set to RGBA4444 and draw a blue
square and then made sure it comes out blue.

[USE s.singapati at gmail.com, 2014-08-08 08:35:20]

Thank you for ideas. Test case writing in progress. We don't have blue.png in
cc/test/data, so, I am creating blue.png. Is there any restrictions about
creating new png? Also, I noticed that renderer_pixeltest.cc is not built for
ANDROID. When it is enabled and run cc_unittests for Android, all
renderer_pixeltest cases are failed. Could you provide any info on hows it is
supposed to be run?

[USE s.singapati at gmail.com, 2014-08-08 12:50:05]

Added pixel test and blue.png

[danakj, 2014-08-08 13:10:39]

On Fri, Aug 8, 2014 at 4:35 AM, <s.singapati@samsung.com> wrote:

> Thank you for ideas. Test case writing in progress. We don't have blue.png
> in
> cc/test/data, so, I am creating blue.png. Is there any restrictions about
> creating new png? Also, I noticed that renderer_pixeltest.cc is not built
> for
> ANDROID. When it is enabled and run cc_unittests for Android, all
> renderer_pixeltest cases are failed. Could you provide any info on hows it
> is
> supposed to be run?
>

It needs OSMesa which may or may not be available on android now.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-08-08 18:16:17]

https://codereview.chromium.org/434963002/diff/20001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/434963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:2118: TYPED_TEST(RendererPixelTest,
PictureDrawQuadTexture4444) {
This isn't relevant for the software renderer, so probably should be
TEST_F(GLRendererPixelTest, PictureDrawQuadTexture4444).

https://codereview.chromium.org/434963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:2121: ResourceFormat texture_format = RGBA_4444;
Do all platforms support this format? Should there be a check and an early out?

https://codereview.chromium.org/434963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:2157: FuzzyPixelOffByOneComparator(true)));
Use an exact comparator.

[USE eero AT chromium.org, 2014-08-11 11:33:58]

Sarath is out of office for a while, so I took over the responsibility to
continue with this issue.

How should I upload my fixes? I am not allowed to upload them directly as I am
not the owner of the issue. Should I create a new issue?

https://codereview.chromium.org/434963002/diff/20001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/434963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:2118: TYPED_TEST(RendererPixelTest,
PictureDrawQuadTexture4444) {
On 2014/08/08 18:16:17, enne wrote:
> This isn't relevant for the software renderer, so probably should be
> TEST_F(GLRendererPixelTest, PictureDrawQuadTexture4444).

Done.

https://codereview.chromium.org/434963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:2121: ResourceFormat texture_format = RGBA_4444;
On 2014/08/08 18:16:17, enne wrote:
> Do all platforms support this format? Should there be a check and an early
out?

No idea. I did not find any relevant function to be used for testing the
support. Is there any?

https://codereview.chromium.org/434963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:2157: FuzzyPixelOffByOneComparator(true)));
On 2014/08/08 18:16:17, enne wrote:
> Use an exact comparator.

Done. For that to work, I changed the blue components of the blue.png image from
254 to 255.

[danakj, 2014-08-11 13:55:41]

On Mon, Aug 11, 2014 at 7:33 AM, <e.hakkinen@samsung.com> wrote:

> Sarath is out of office for a while, so I took over the responsibility to
> continue with this issue.
>
> How should I upload my fixes? I am not allowed to upload them directly as
> I am
> not the owner of the issue. Should I create a new issue?


Yep make a new one.


>
>
>
> https://codereview.chromium.org/434963002/diff/20001/cc/
> output/renderer_pixeltest.cc
> File cc/output/renderer_pixeltest.cc (right):
>
> https://codereview.chromium.org/434963002/diff/20001/cc/
> output/renderer_pixeltest.cc#newcode2118
> cc/output/renderer_pixeltest.cc:2118: TYPED_TEST(RendererPixelTest,
> PictureDrawQuadTexture4444) {
> On 2014/08/08 18:16:17, enne wrote:
>
>> This isn't relevant for the software renderer, so probably should be
>> TEST_F(GLRendererPixelTest, PictureDrawQuadTexture4444).
>>
>
> Done.
>
>
> https://codereview.chromium.org/434963002/diff/20001/cc/
> output/renderer_pixeltest.cc#newcode2121
> cc/output/renderer_pixeltest.cc:2121: ResourceFormat texture_format =
> RGBA_4444;
> On 2014/08/08 18:16:17, enne wrote:
>
>> Do all platforms support this format? Should there be a check and an
>>
> early out?
>
> No idea. I did not find any relevant function to be used for testing the
> support. Is there any?
>
>
> https://codereview.chromium.org/434963002/diff/20001/cc/
> output/renderer_pixeltest.cc#newcode2157
> cc/output/renderer_pixeltest.cc:2157:
> FuzzyPixelOffByOneComparator(true)));
> On 2014/08/08 18:16:17, enne wrote:
>
>> Use an exact comparator.
>>
>
> Done. For that to work, I changed the blue components of the blue.png
> image from 254 to 255.
>
> https://codereview.chromium.org/434963002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[USE eero AT chromium.org, 2014-08-11 14:29:34]

On 2014/08/11 13:55:41, danakj wrote:
> Yep make a new one.

It is here: https://codereview.chromium.org/460563003/