Update Skia to milestone 66

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: lsalzman, Assigned: lsalzman)

Tracking

(Blocks 1 bug)

56 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 60+, firefox60 fixed, firefox61 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

We're still on milestone 59. Milestone 66 is already out, so it is about time we updated.
A lot of fixes here to deal with code churn.

Skia no longer fully allocates the padding on the last row, so in some cases I have to do that manually now.
LayerRasterizer is gone, so I had to rewrite Mask().
Some churn in GrGL interface.
Other miscellaneous little things.
Attachment #8957672 - Flags: review?(jmuizelaar)
I needed to do a few fixes to deal with their changing gn setup, but otherwise mostly just file churn.
Attachment #8957673 - Flags: review?(jmuizelaar)
A few things:

The android emulator does not actually support BGRA8888, so reporting it breaks things. Thusly, we should no longer report it to Skia.

Some GL types and function prototypes had to be matched to more closely match both the khronos platform headers and Skia's GL headers with more closely match khronos'. I checked on all platforms, and our header was the odd man out on definitions of GLboolean/GLintptr/GLsizeiptr and a prototype or two. Otherwise, there were type errors with SkiaGL's function pointer interface.

Some new function pointers that SkiaGL is using.
Attachment #8957675 - Flags: review?(jmuizelaar)
Posted patch wrap STL mutex header (obsolete) — Splinter Review
Skia depends on using std::mutex in a few cases, so we need to expose this.
Attachment #8957676 - Flags: review?(mh+mozilla)
VideoWidth/VideoHeight can sometimes return 0, which only happens when the video source is invalid. In that case, we'd actually supply source width/height that was Inf and/or NaN, totally screwing up the rendering.

Skia very much doesn't like this anymore and crashes. Therefor, we no longer torture DrawTargetSkia with these non-finite FP values.
Attachment #8957679 - Flags: review?(jmuizelaar)
Mostly harmless fuzz churn. Some rendering results changes by 1 increment here or there.
Attachment #8957683 - Flags: review?(jmuizelaar)
Posted file Update to Skia m66
Updated sources for moz-skia now in my moz-m66 branch.

The main things I had to change here is disabling their new Delta AA for now, which mostly seems broken for our use-cases.

Workarounds for some macOS driver bugs.

Re-enabling some deprecated clipping features.

Had to add support for SkDevice snapshots to support our PushLayer/PopLayer setup until I can properly vet the functionality/performance of their new layer masking support.

Had to backport SkConvolver/SkBitmapScaler support which our imagelib depends on, but which they decided to remove.

Various build fixes to work across all our compiler toolchains and get past static analysis.

... And otherwise just fixing a metric ton of bitrot with our other patches.
Attachment #8957685 - Flags: review?(jmuizelaar)
Comment on attachment 8957672 [details] [diff] [review]
Moz2D fixes for Skia m66 update

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +401,5 @@
>  
>  static bool
>  ExtractAlphaBitmap(const sk_sp<SkImage>& aImage, SkBitmap* aResultBitmap)
>  {
>    SkImageInfo info = SkImageInfo::MakeA8(aImage->width(), aImage->height());

Add a comment about why we're doing this stride exercise here.
Attachment #8957672 - Flags: review?(jmuizelaar) → review+
Attachment #8957673 - Flags: review?(jmuizelaar) → review+
Attachment #8957675 - Flags: review?(jmuizelaar) → review+
Attachment #8957679 - Flags: review?(jmuizelaar) → review+
Attachment #8957683 - Flags: review?(jmuizelaar) → review+
Attachment #8957685 - Flags: review?(jmuizelaar) → review+
Attachment #8957676 - Attachment is obsolete: true
Attachment #8957676 - Flags: review?(mh+mozilla)
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84a6ea4264ca
part 1 - update to Skia m66. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6b0bafaab4
part 2 - Moz2D fixes for Skia m66 update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/12dba47d2024
part 3 - mozbuild fixes for Skia m66 update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/614317354b14
part 4 - GL fixes for Skia m66 update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73b781388a8
part 5 - don't attempt to draw an invalid video source in canvas. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3410beb9eb1
part 6 - fuzz for Skia m66 update. r=jrmuizel
Big perf improvements won! \o/

== Change summary for alert #12083 (as of Mon, 12 Mar 2018 18:38:01 GMT) ==

Improvements:

154%  rasterflood_gradient linux64 pgo e10s stylo     288.58 -> 733.25
137%  rasterflood_gradient linux64 opt e10s stylo     316.83 -> 750.58
 10%  tsvgx linux64 pgo e10s stylo                    208.25 -> 187.19
 10%  tsvgx linux64 opt e10s stylo                    215.02 -> 194.11
  4%  tscrollx linux64 opt e10s stylo                 0.87 -> 0.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12083
there is one regression that came from this:
 	tsvgx windows7-32 opt e10s stylo  (untriaged)   graph · subtests 	452.97 	< 	478.02 	5.53%

you can see all the other wins:
https://treeherder.mozilla.org/perf.html#/alerts?id=12083

what is interesting is on autoland an improvement landed about the same time and when it all merged we end up with the same results.

:lsalzman, I know win7 runs in different graphics modes than the other OS flavors, is this concerning to you at all or should we accept it?  it is 5%+
Flags: needinfo?(lsalzman)
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #14)
> there is one regression that came from this:
>  	tsvgx windows7-32 opt e10s stylo  (untriaged)   graph · subtests 	452.97 
> < 	478.02 	5.53%
> 
> you can see all the other wins:
> https://treeherder.mozilla.org/perf.html#/alerts?id=12083
> 
> what is interesting is on autoland an improvement landed about the same time
> and when it all merged we end up with the same results.
> 
> :lsalzman, I know win7 runs in different graphics modes than the other OS
> flavors, is this concerning to you at all or should we accept it?  it is 5%+

I can look into this and see if there is anything I can do with it today/tomorrow. Regardless, we can't backout the Skia update overall over this, since we're already a year out of date with upstream Skia, and for security reasons and just avoiding the forking of Skia, we need to take the update.

None the less, as noted, I will investigate what's going on and try to solve that since the same subtests show some regressions even on Linux, but to a smaller degree to where they don't poison the summary score.
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9064f0a3fb62
follow-up - support BGRA in bilerp_clamp_8888 fast-path. r=me
Okay, that should take care of the performance regression to some extent. We fell off a fast-path in some of their new rasterization code. I added an easy workaround that routes us back into it and puts the tsvgx summary scores at a net-positive again on Win7.
Flags: needinfo?(lsalzman)
Depends on: 1447035
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2ae2186dbc9
follow-up - disable explicit GPU resource allocation on all platforms. r=dholbert
Depends on: 1447913
Depends on: 1448189
New perf improvements since comment 20:

== Change summary for alert #12310 (as of Thu, 22 Mar 2018 05:53:57 GMT) ==

Improvements:

  6%  tsvgx windows7-32 opt e10s stylo     461.62 -> 434.07
  6%  tsvgx windows7-32 pgo e10s stylo     395.45 -> 372.32

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12310
Depends on: 1457284
Depends on: 1462868
Depends on: 1474141
Depends on: 1483120
You need to log in before you can comment on or make changes to this bug.