1055265 - Flash fails because SkBitmapDevice doesn't want to work with kRGBA_8888_SkColorType

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Plugins, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

I am not really sure what happened here, but it looks like we are not able to create a SkBitmap-backed canvas with RGBA_8888 anymore. Flash uses this as part of the ANP canvas API.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix pixel format issue with Flash

This patch fixes it but I don't really understand why. kN32_SkColorType appears to have different values in SkANP.cpp vs SkBitmapDevice.cpp so this change makes it pass the assert in SkBitmapDevice. I really expected to the red and green channels swapped on the screen, but that doesn't appear to be happening?

Comment 2

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8474787 [details] [diff] [review]
Fix pixel format issue with Flash

Review of attachment 8474787 [details] [diff] [review]:
-----------------------------------------------------------------

Well this is odd. Do we know what's going on here?

Comment 3

[George Wright (:gw280) (needinfo me!)]

OK, so it looks like basically Skia sets kN32_ColorType to either BGRA or RGBA depending on what endianness it was built with. I think the most correct thing we can do here is to set colorType to kN32_ColorType and let Skia just choose the right one for us.

Comment 4

[George Wright (:gw280) (needinfo me!)]

Attachment: colours.patch

Let skia choose the correct byte order for us based on what endianness it was built with. This ensures that we always pass the assertion no matter what architecture we're on.

As for the colours, I suspect that the ANP enum type is just "this is a 32-bit pixel format in whatever order makes sense on the current architecture", and that's why we're not seeing R and B swapped?

Comment 5

[George Wright (:gw280) (needinfo me!)]

Attachment: colours.patch

Ensure the skia byteorder is consistent amongst different areas of the codebase as well

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8480212 [details] [diff] [review]
colours.patch

Review of attachment 8480212 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, that explains why we had the discrepancy. Thanks.

Comment 7

[George Wright (:gw280) (needinfo me!)]

Attachment: colours-final.patch

Final version of the patch. It stops the crash, but nothing's rendering?

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8480718 [details] [diff] [review]
colours-final.patch

Review of attachment 8480718 [details] [diff] [review]:
-----------------------------------------------------------------

works here, so I'm happy

Comment 9

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fba966c2cd07

Comment 10

[Jan Henning [:JanH]]

Flash is going away (bug 1381916), so there's no point in pursuing this further.