I introduced a bug that causes us never to use accelerated Skia GL canvas.

Ritu, you should be aware of this issue.

If I understand correctly, it causes performance regressions on non-Windows system (including Android)

I don't think we can/should block the 41 release as this regression is pretty old (Bas said most of the 41 cycle). This has not been tested enough. However, we should mention that in the release notes in the know issues. Release Note Request (optional, but appreciated) [Why is this notable]: Might impact some games [Suggested wording]: Performance regression on 2D canvas (1206076) [Links (documentation, blog post, etc)]: This bug

I can confirm this causes us to use SkiaGL on OS X

Brad, there is a performance regression in 41 with canvas on Android. How far do you want the fix uplifted?

Sotaro, can you check with Bas if we still have B2G try run failures, and help take care of them? We want to enable SkiaGL canvas on mobile platforms as soon as possible (where it's a regression that we're not using it.)

(In reply to Milan Sreckovic [:milan] from comment #8) > Brad, there is a performance regression in 41 with canvas on Android. How > far do you want the fix uplifted? Probably too late for 41 unfortunately, let's get this into 42.

The checked-in patch sets gfx.canvas.azure.accelerated to false in two places. I don't see that in the attached patch or discussed in the bug. Was it intentional?

(In reply to Jesse Ruderman from comment #12) > The checked-in patch sets gfx.canvas.azure.accelerated to false in two > places. I don't see that in the attached patch or discussed in the bug. Was > it intentional? Yes, re-enabling acceleration on those platforms caused a bunch of test failures, on one of those platforms it was never enabled in the first place. Setting the pref to false allowed us to re-activate acceleration on Android and remove the confusion as to whether acceleration is enabled or disabled on a platform.

Based on comment 10, this is a wontfix for 41. Tracked for 42+.

Bas, are you planning to uplift the patch to 42? Thanks

Comment on attachment 8662925 [details] [diff] [review] Use a specialized PersistentBufferProvider for Canvas2D when using a SkiaGL DrawTarget. Approval Request Comment [Feature/regressing bug #]: 1167235 [User impact if declined]: Unaccelerated Canvas on Android [Describe test coverage new/current, TreeHerder]: Aurora and nightly testing [Risks and why]: Relatively low, a lot of testing and this code used to work [String/UUID change made/needed]: None

I don't think this has landed on aurora yet. Bas can you also request uplift so it gets into 43? Thanks.

Comment on attachment 8662925 [details] [diff] [review] Use a specialized PersistentBufferProvider for Canvas2D when using a SkiaGL DrawTarget. [Triage Comment] OK, let's try that.

RE uplifts -- this bug caused a graphical regression on Nightly, on a fairly prominent site (forecast.io). See bug 1210444 and its attached screencast. Does that influence our calculus about whether we actually want this on branches? (Might want to back out comment 19 and/or 20.)

(In reply to Daniel Holbert [:dholbert] from comment #21) > RE uplifts -- this bug caused a graphical regression on Nightly, on a fairly > prominent site (forecast.io). See bug 1210444 and its attached screencast. > > Does that influence our calculus about whether we actually want this on > branches? (Might want to back out comment 19 and/or 20.) I think we still want to uplift, we just want to turn the pref off on Android :-). Better to have it working but preffed off, but preffed on and not doing anything :-).

That makes sense, sure. (RE rethinking the uplifts -- I was mostly talking about the pref-enabling part of this patch.) Could you take care of that? (landing on trunk & uplifting a pref-disabling patch, as a followup to comment 19 & comment 20's landings -- either here or on a separate bug, whichever makes the most sense) Then, we (you? :D) can investigate bug 1210444 with a bit less urgency (and with a local pref-enable to make the bug visible).

Are you reopening here & backing this out, or filing a new bug to flip the pref?

(In reply to Daniel Holbert [:dholbert] from comment #23) > That makes sense, sure. (RE rethinking the uplifts -- I was mostly talking > about the pref-enabling part of this patch.) > > Could you take care of that? (landing on trunk & uplifting a pref-disabling > patch, as a followup to comment 19 & comment 20's landings -- either here or > on a separate bug, whichever makes the most sense) > > Then, we (you? :D) can investigate bug 1210444 with a bit less urgency (and > with a local pref-enable to make the bug visible). Not me, don't have an android device, I'll leave that to Milan to decide ;-). But yeah, I'll take care of that.

(In reply to Daniel Holbert [:dholbert] from comment #23) > > Then, we (you? :D) can investigate bug 1210444 with a bit less urgency (and > with a local pref-enable to make the bug visible). One possibility of the flickering is that SurfaceFactory::NewTexClient() recycles SharedSurfaceTextureClient, but it seems not correctly recycled except gonk. On another platform, SharedSurfaceTextureClient seems recycled too early.

Relnote nomination for this one does not make sense anymore as 41 will be end of life'd tomorrow.

(In reply to Ritu Kothari (:ritu) from comment #27) > Relnote nomination for this one does not make sense anymore as 41 will be > end of life'd tomorrow. It was first fixed for 42 (not 41), does it make sense to still add the relnote in 42 to highlight it?

Redirecting to Sylvestre as he owns 42.

Ryan, you would like to add something like "Accelerated Skia GL canvas was not activated" as a Fixed Issues for Android. Correct?

(In reply to Sylvestre Ledru [:sylvestre] from comment #30) > Ryan, you would like to add something like "Accelerated Skia GL canvas was > not activated" as a Fixed Issues for Android. Correct? Yes, that's what I was thinking. Also, according to comment 2, it also affects non-Windows desktop systems, so it seems both desktop and Android notes should be updated. I am mostly just an observer here though, so :bas.schouten is probably best to ask if we need confirmation.