Closed Bug 1468801 Opened 2 years ago Closed 2 years ago

Disable SkiaGL

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- verified
firefox63 --- verified

People

(Reporter: lsalzman, Assigned: lsalzman)

References

(Regressed 3 open bugs)

Details

(Whiteboard: [geckoview:klar:p2])

Attachments

(1 file)

SkiaGL is only used to accelerate Canvas2D, and only on Android and macOS by default. It exposes us to a large amount of extra code and churn from Skia upstream.

If we just relied on software Skia instead, we would avoid this problem and simplify Skia maintenance.

Software Skia performance is competitive and on some benchmarks (i.e. CanvasMark) our performance is actually better in software than with SkiaGL.
Pref Skia off as a preliminary step before we remove any code.
Attachment #8985427 - Flags: review?(snorp)
I worries that it hugely decrease fps of some website like testdrive-archive.azurewebsites.net/performance/fishbowl See Bug 1411481.
We might be interested on the perf impact of real websites, but we aren't interested in benchmarks which really should be written in WebGL.
Attachment #8985427 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/2719ceb0166e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Can we uplift this to Beta? We're seeing bug 1472356 in Focus (which uses the Beta branch of GeckoView) that is fixed by this bug .
Flags: needinfo?(lsalzman)
To avoid the uplift risk for macOS, we could uplift with an Android #ifdef.
Whiteboard: [geckoview:klar:p2]
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> Can we uplift this to Beta? We're seeing bug 1472356 in Focus (which uses
> the Beta branch of GeckoView) that is fixed by this bug .

That puts us in a bind, because if we use this as a "fix" for SkiaGL bugs, then that means we're committed to not backing this out, regardless of any potential performance impacts it has. If we did for some reason backout the SkiaGL deprecation, then suddenly all the bugs that were "fixed" by getting rid of SkiaGL come raging back. 

But given that we've been living with this for 3 weeks so far in nightly with things having been quiet, and there is sufficient time left in beta, it could make sense to uplift.
Flags: needinfo?(lsalzman)
Yeah. I guess the other choice is to actually fix the SkiaGL bug on Beta, which I'm not sure we have the resources to do at the moment. I think uplifting this bug is the way to go.
Flags: needinfo?(lsalzman)
Comment on attachment 8985427 [details] [diff] [review]
deprecate SkiaGL for Canvas2D

Approval Request Comment
[Feature/Bug causing the regression]: Pre-existing condition.
[User impact if declined]: Buggy OpenGL drivers on Android preventing Google Maps from working.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: not very
[Why is the change risky/not risky?]: Since this disables OpenGL-accelerated canvas, we will no longer have to deal with OpenGL driver bugs. Instead, we just use the same reliable software renderer we use on Windows and Linux which, while possibly being somewhat slower, at least should always work.
[String changes made/needed]: none
Flags: needinfo?(lsalzman)
Attachment #8985427 - Flags: approval-mozilla-beta?
Comment on attachment 8985427 [details] [diff] [review]
deprecate SkiaGL for Canvas2D

Let's give this a try on beta. From discussion with Lee, this was intended to land in 62 nightly. If we see issues on MacOS in 62 or 63, we can flip the pref back for Mac and leave it in place for mobile (as cpeterson suggested).
Attachment #8985427 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
We can do a spotcheck here on macOS using some Canvas demos to make sure that this did not caused any issues.
Flags: qe-verify+
I verified this issue on Mac OS X 10.12 with FF Nightly 63.0a1(2018-07-19) and FF beta 62.0b10 using some test pages that can be found here: http://www.kevs3d.co.uk/dev/, https://html5demos.com/canvas/, google maps. I didn't spot any issues during this verification, based on that I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1335014
See Also: → 1437556
Did anyone benchmark Vulkan backend for Skia? See https://skia.org/user/special/vulkan

I opened a feature request to enable it (bug 1502740).
Blocks: 1530471

to be honest, I actually don't get this change.

Blit-heavy canvas workloads suffer a lot by the decision to perform software-only rendering (justr look at the fishbowl results representative for so many canvas-based games).

There were times when browser-vendors (including mozilla) were proud to have hardware accelerated canvas - now it is disbled because of a few broken android drivers.

(In reply to Clemens Eisserer from comment #18)

to be honest, I actually don't get this change.

Blit-heavy canvas workloads suffer a lot by the decision to perform software-only rendering (justr look at the fishbowl results representative for so many canvas-based games).

There were times when browser-vendors (including mozilla) were proud to have hardware accelerated canvas - now it is disbled because of a few broken android drivers.

For blit-heavy workloads, you will get better performance, availability, flexibility, and control with WebGL. There are a number of popular JS libraries to help work with WebGL and ease the transition. This is the direction that games especially and the web in general are moving.

For other workloads, the spotty availability, instability, and security concerns are a major detractor for SkiaGL Canvas2D for various business or productivity oriented apps. Better stability and reliability are a boon there. Also, for text and filter intensive workloads, performance in software is actually better than the SkiaGL accelerated canvas (famously or infamously in CanvasMark).

Since Firefox is moving to WebRender (hopefully soon?) which isn't going to be using Skia either way, this is not a big deal really.

Regressions: 1590515
Regressions: 1597937
Regressions: 1602299
You need to log in before you can comment on or make changes to this bug.