960276 - SkiaGL incorrectly swizzles BGRA textures leaving a visual line in the middle of the texture

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

In bug 951241, it sounds like we determined that on some hardware, SkiaGL has artifacts when drawing tiled bitmaps. In that case, a blue line.

Comment 2

[Jason Smith [:jsmith]]

After confirming now that this has reproduced in three locations (two which have workarounds), I think we have no choice but to fix this. It's very evident to me that working around the bug here isn't a good idea - this problem keeps popping up in a new location after doing more & more exploratory testing. Noming, as we need the real fix.

Comment 3

[Preeti Raghunath(:Preeti)]

Milan

Please take a look. 1.3+

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

I agree it should be fixed.  We're going to look at it with the latest Skia update on 1.4.  Once we have a fix there, we'll see if it can be uplifted and applied to 1.3 which has an older Skia version.

Comment 6

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

Do we have a confirmed test case for this issue?

Comment 7

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

(In reply to George Wright (:gw280) from comment #6)
> Do we have a confirmed test case for this issue?

I don't think we do, we should write one. My understanding is that it should be triggered by drawing a Large image into the canvas with scaling.

Comment 8

[Jason Smith [:jsmith]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #7)
> (In reply to George Wright (:gw280) from comment #6)
> > Do we have a confirmed test case for this issue?
> 
> I don't think we do, we should write one. My understanding is that it should
> be triggered by drawing a Large image into the canvas with scaling.

FWIW - I know we've seen this happen in the following three locations:

1. Editing a photo in the gallery (currently has a workaround implemented)
2. Zooming in on a contact's photo in the contacts app (currently has a workaround implemented)
3. Setting a wallpaper on the homescreen (no workaround implemented)

Comment 9

[Jason Smith [:jsmith]]

Using bug 951241 as a reference, let's a get a regression window. That means start from the 12/17/2013 1.3 build and work your way backwards to find the gecko regression range.

Comment 10

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

I have confirmed this was fixed by bug 910754 landing in trunk, but it's not eligible for uplift to 1.3 branch. I am searching for the exact fix for the issue now inside Skia.

Comment 12

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

So here are my findings so far:

Skia r13442 (currently in m-c) doesn't have this bug
Skia r11115 doesn't have this bug
Skia r8495 (currently in 1.3) has this bug

I can't bisect any further without sinking lots of time, as there's a separate bug that manifested in between r8495 and r11115 that means that images don't render at all, so I can't test this testcase to see whether it passes/fails.

As a result of the urgency of this bug I'm going to switch tactics to tracing through the code directly and seeing if I can get it sorted in the next couple of days.

Comment 13

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

Further update:

Drilling down into Skia's bitmap rendering code I don't see anything wrong with it. I also found out something interesting - if I hardcode Skia to use RGBA8888 instead of BGRA8888, the issue is no longer present, so that suggests there's likely a driver bug at play here as the colour format shouldn't be affecting anything other than the R/B channels being swapped (in this case)?

Further, I ran it through apitrace and it slows everything right down, and I can actually see the image rendering on screen in two halves - the top half, and the bottom half. I don't think Skia is doing any tiling here based on what I've been observing, so either Gecko or the driver is tiling somehow. The line appears between these two halves, which suggests to me that something is broken with dealing with edge cases in the BGRA case, but which works fine in the RGBA case.

I'm continuing to trace through Skia's OpenGL calls to see exactly which call it is that causes things to magically start working when using RGBA.

Comment 14

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

Update: not a driver bug. I know what it is. Patch incoming.

Comment 15

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

Attachment: .0001-Bug-960276-Ensure-that-we-swizzle-the-middle-row-cor.patch.swp

Proposed fix

Comment 16

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

Attachment: 0001-Bug-960276-Ensure-that-we-swizzle-the-middle-row-cor.patch

Maybe this time I upload the patch instead of a vim swap file?

Comment 17

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

Comment on attachment 8385649 [details] [diff] [review]
0001-Bug-960276-Ensure-that-we-swizzle-the-middle-row-cor.patch

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

Great catch.

Comment 18

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

Comment on attachment 8385649 [details] [diff] [review]
0001-Bug-960276-Ensure-that-we-swizzle-the-middle-row-cor.patch

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): busted drawImage on canvas with odd-heighted images
User impact if declined: visual crappiness
Testing completed: locally, verified the testcase in bug 951241 works now
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 19

[Roland Kunkel [:RolandK]]

I cannot reproduce this issue with the three mentioned methods in the latest v1.4 build.

3/5/2014
Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140305040204
Gaia: 6781459a49642ca0eb7ec3e95667808d5d77b656
Gecko: e5b09585215f
Version: 30.0a1
Firmware Version: V1.2-device.cfg

Based on the above comments changing status to verified fixed.

Comment 20

[Jason Smith [:jsmith]]

This has not landed, so that means you did not test this correctly. Reopening.

Comment 21

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

George we're gonna want this patch in master/1.4 too, just because it could cause problems with other usage.

Comment 22

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

And actually could you please upstream it too? You're the best.

Comment 23

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

This bug doesn't appear to affect upstream, because they fixed it by refactoring, so 1.4 should be unaffected :)

Comment 24

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

(By which I mean they've gone completely insane/awesome and they now do all their swizzling using a shader on the GPU. Woo!)

Comment 25

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

(In reply to Jason Smith [:jsmith] from comment #20)
> This has not landed, so that means you did not test this correctly.
> Reopening.

It looks like the verification was done against 1.4, which is known to be unaffected by this bug as it has a newer Skia revision in it.

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 8385649 [details] [diff] [review]
0001-Bug-960276-Ensure-that-we-swizzle-the-middle-row-cor.patch

Approving this for 1.3 also requesting verification once this lands on the 1.3 branch.

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

This needs to be landed on 1.3 branch only.

Comment 28

[Sandip Kamat]

PM Triage - Must fix for 1.3 (leave as 1.3+)

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5202e96c6cb5

Comment 30

[Roland Kunkel [:RolandK]]

> FWIW - I know we've seen this happen in the following three locations:
> 
> 1. Editing a photo in the gallery (currently has a workaround implemented)
> 2. Zooming in on a contact's photo in the contacts app (currently has a
> workaround implemented)
> 3. Setting a wallpaper on the homescreen (no workaround implemented)

Verified as fixed v1.3. This issue does NOT reproduce in the latest v1.3 Buri build:

3/26 Environmental Variables:
Device: Buri 1.3 MOZ RIL
BuildID: 20140326004002
Gaia: 812838ad0fabf51fa14435af562ddac6d26fa936
Gecko: ba97efb0da4b
Version: 28.0
Firmware Version: V1.2-device.cfg

-

1) Leaving verifyme keyword as this needs to be verified on the v1.3T branch.

Comment 31

[Roland Kunkel [:RolandK]]

Verified as fixed v1.3T. This issue does NOT reproduce in the latest v1.3 Tarako build:

1.3T Environmental Variables:
Device: Tarako 1.3T MOZ RIL
BuildID: 20140407004001
Gaia: d9ac78148591a50356dd5a82d2d295eeb7c234a6
Gecko: f105ec652173
Version: 28.1
Firmware Version: sp8810

-

1) Verified on all branches, changing status to verified.