Closed Bug 1468801 Opened 2 years ago Closed 2 years ago
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+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2719ceb0166e deprecate SkiaGL for Canvas2D. r=snorp
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 .
To avoid the uplift risk for macOS, we could uplift with an Android #ifdef.
(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.
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.
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
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.
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.
Did anyone benchmark Vulkan backend for Skia? See https://skia.org/user/special/vulkan I opened a feature request to enable it (bug 1502740).
11 months ago
Depends on: 1529614
You need to log in before you can comment on or make changes to this bug.