bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

Update Skia to m51 branch

RESOLVED FIXED in Firefox 49



2 years ago
a year ago


(Reporter: lsalzman, Assigned: lsalzman)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, relnote-firefox -)


(Whiteboard: [gfx-noted])


(5 attachments)



2 years ago
Skia recently tagged their chrome/m51 branch. It incorporates many of the bug-fixes and improvements we've submitted upstream, so it should cut down on some of our local patch maintenance load. This branch also moves further along in integrating C++11 features, so we want to keep up to date with it for the sake of sanity and not letting our tree diverge so much that updates start becoming a headache. The proposed timeline for deploying this is with our 49 release, after the train flip from 48.

Comment 1

2 years ago
Created attachment 8742008 [details] [diff] [review]
part 1 - update moz.build for Skia m51

I mainly just had to clear out some opt files that Skia no longer provides and also isolate some new source files outside of unified sources to deal with conflicts.
Attachment #8742008 - Flags: review?(jmuizelaar)

Comment 2

2 years ago
Created attachment 8742009 [details] [diff] [review]
part 2 - update SkiaGLGlue for Skia m51

Skia got rid of the per-GL-function callback, as they intend us to now use functionals to deal with such things. Rather than raw function pointers in the GL interface, they now accept std::function.

This patch thus cleans up the old callback code to build a lambda that does the MakeCurrent() handling we were doing before, but now without needing to use any thread-local storage for the context global to communicate between the callback and the function pointer. Overall, this seems much cleaner than what we were doing before.

Also one little oddity where I had to make sure to null out the functions when SkiaGLGlue is destroyed to prevent them from keeping the context alive, but that none the less maintains the previous status quo that after SkiaGLGlue is destroyed, the GrGL interface was in an invalid/non-usable state, but now at least a bit more intentionally.
Attachment #8742009 - Flags: review?(jmuizelaar)

Comment 3

2 years ago
Created attachment 8742010 [details] [diff] [review]
part 3 - update Moz2d for Skia m51

The main changes here deal with the use of sk_sp, Skia's new shared pointer, which it uses in many places where before you would use explicit SkRef or SkAutoTUnref. Seems like a positive API change from upstream.
Attachment #8742010 - Flags: review?(jmuizelaar)

Comment 4

2 years ago
Created attachment 8742011 [details] [diff] [review]
part 4 - fix tests for Skia m51 update

Upstream made some changes to how zero-length paths are stroked when different line cap/join settings are in play. This causes us to, if looked at in a positive way, now fail some tests consistently in the same way that all our other backends do... so Skia becomes a bit less special. :)

Also needed a tiny bit more fuzz on a shadow-blur related test. One extra pixel off...
Attachment #8742011 - Flags: review?(jmuizelaar)
Comment on attachment 8742010 [details] [diff] [review]
part 3 - update Moz2d for Skia m51

Review of attachment 8742010 [details] [diff] [review]:

sk_sp is a nice improvement.
Attachment #8742010 - Flags: review?(jmuizelaar) → review+
Attachment #8742008 - Flags: review?(jmuizelaar) → review+
Attachment #8742011 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8742009 [details] [diff] [review]
part 2 - update SkiaGLGlue for Skia m51

Review of attachment 8742009 [details] [diff] [review]:

::: gfx/gl/SkiaGLGlue.cpp
@@ +30,4 @@
>  {
> +  return [aContext, aFunc] (A... args) -> R
> +  {
> +    aContext->MakeCurrent();

It is sad that we call this on every gl call...

@@ +210,5 @@
> +    i->fFunctions.fGenTextures = WrapGL(context, &GLContext::fGenTextures);
> +    i->fFunctions.fGenerateMipmap = WrapGL(context, &GLContext::fGenerateMipmap);
> +    i->fFunctions.fGetBufferParameteriv = WrapGL(context, &GLContext::fGetBufferParameteriv);
> +    i->fFunctions.fGetError = WrapGL(context, &GLContext::fGetError);
> +    i->fFunctions.fGetIntegerv = getIntegerv;

Can you add a comment about why getIntegerv is special?
Attachment #8742009 - Flags: review?(jmuizelaar) → review+

Comment 7

2 years ago
Created attachment 8746695 [details]
part 5 - update Skia to m51 branch

Just linking here to the moz-skia m51 branch repository since the actual patch against the tree is too cumbersome. Check the commits at the top for the actual modifications made over Skia. The main patch that changed was "replace C++11 standard library usage with MFBT". Had to change some bits on other patches to make them apply again, and otherwise cull out a lot of old patches that were upstreamed or unnecessary now.
Attachment #8746695 - Flags: review?(jmuizelaar)
Attachment #8746695 - Flags: review?(jmuizelaar) → review+


2 years ago
Depends on: 1270231
this seemed to cause num_constructors to increase as well as android apk size:
Flags: needinfo?(lsalzman)


2 years ago
Blocks: 1271356


2 years ago
Flags: needinfo?(lsalzman)
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Skia graphics library has been updated to version 51.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Relnote - for the user notes, not sure this would be helpful in the general notes.
relnote-firefox: ? → -
Blocks: 1276161


2 years ago
See Also: → bug 1286213
Depends on: 1306883
Depends on: 1315848
You need to log in before you can comment on or make changes to this bug.