1299435 - Update Skia to m55 branch

Status: VERIFIED FIXED

(Core :: Graphics, enhancement, P1)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

With Project Backendnuken (bug #1186552) getting real on Windows (bug #1007702) and also Skia being enabled by default as content backend on Linux (bug #1278957), updating Skia to stable m54 branch will be a nice addition.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

m55 is now a stable branch since 2016-10-06.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: Bug 1299435 - part 1 - fix Skia moz.build for Skia m55 update

This concerns mainly replacing our usage of SkBitmap with SkImage. In the process of this, due to deficiencies in the Skia API that I am still in the process of negotiating around with upstream, we had to use some internal APIs from their src/ dirs instead of merely the public include/ dirs. Nothing too nasty, though, and will hopefully disappear in future updates.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: Bug 1299435 - part 2 - fix Moz2d for Skia m55 update

Oops, mixup, see comment for part 1 patch for part 2 details. :)

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: Bug 1299435 - part 3 - fix Skia npapi for Skia m55 update

Tweaking some old NPAPI code that would no longer build due to API changes.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: Bug 1299435 - part 5 - adjust fuzz for Skia m55 update

Some small fuzz fixups, mostly related to changes in the SkiaGL backend. Due to some slight breakage with SkiaGL in e10s (as in, e10s no longer reliably enables SkiaGL despite prefs), I had to mark a couple tests as random-if as a short-term workaround.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: Bug 1299435 - part 4 - update Skia source to m55

Changes to Skia source itself to make upstream chrome/m55 branch work with us. The main changes are addition of ARM NEON run-time detection, since upstream decided to get rid of it, a few other performance fixes that only pertain to our quirky build setup, some config changes, and a bunch of little updates to older patches just to fix bit rot due to API changes. See github repo commit logs for more detail.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8803013 [details] [diff] [review]
Bug 1299435 - part 1 - fix Skia moz.build for Skia m55 update

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

::: gfx/skia/generate_mozbuild.py
@@ +99,5 @@
> +    SOURCES['skia/src/opts/SkBlitRow_opts_SSE2.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=20']
> +    SOURCES['skia/src/opts/SkOpts_ssse3.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=31']
> +    SOURCES['skia/src/opts/SkOpts_sse41.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=41']
> +    SOURCES['skia/src/opts/SkOpts_sse42.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=42']
> +    SOURCES['skia/src/opts/SkOpts_avx.cpp'].flags += ['-DSK_CPU_SSE_LEVEL=51']

yay for AVX!

@@ +472,5 @@
>    write_list(f, 'SOURCES', sources['neon'], 8)
>    write_cflags(f, sources['neon'], 'neon', "CONFIG['NEON_FLAGS']", 8)
> +  f.write("    if CONFIG['CPU_ARCH'] == 'aarch64' or CONFIG['BUILD_ARM_NEON']:\n")
> +  write_cflags(f, sources['neon'], opt_whitelist, 'skia_opt_flags', 8)
> + 

nit: whitespace

Comment 8

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8803016 [details] [diff] [review]
Bug 1299435 - part 5 - adjust fuzz for Skia m55 update

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

::: layout/reftests/canvas/reftest.list
@@ +65,5 @@
>  
>  # azure quartz uses CGDrawLinearGradient instead of DrawShading
>  # so we have less control over degenerate behaviour as tested by this
>  # test
> +fails-if((azureSkia&&!azureSkiaGL)||azureQuartz) random-if(azureSkiaGL) == linear-gradient-1a.html linear-gradient-1-ref.html

Are these the only random-ifs because of skiagl in e10s? If so, can we make this e10s only and add a comment above please?

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8803014 [details] [diff] [review]
Bug 1299435 - part 2 - fix Moz2d for Skia m55 update

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

::: gfx/2d/Factory.cpp
@@ +383,5 @@
>        RefPtr<DrawTargetSkia> newTarget;
>        newTarget = new DrawTargetSkia();
> +      if (newTarget->Init(aData, aSize, aStride, aFormat, aUninitialized)) {
> +        retVal = newTarget;
> +      }

nit: gfxCriticalError if we can't make the draw target

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +76,5 @@
> +        aFormat :
> +        SkiaColorTypeToGfxFormat(pixmap.colorType(), pixmap.alphaType());
> +    mStride = pixmap.rowBytes();
> +  } else if (aFormat != SurfaceFormat::UNKNOWN) {
> +    mFormat = aFormat;

Should we be worried here if peekPixels actually fail? Why would this happen?

@@ +131,5 @@
> +    SkPixmap pixmap;
> +    if (mImage->peekPixels(&pixmap)) {
> +      mImage = SkImage::MakeRasterCopy(pixmap);
> +      if (!mImage) {
> +        gfxCriticalError() << "Failed copying Skia raster snapshot";

nit: Should also check if mImage->peekPixels returns false also.

Comment 10

[Lee Salzman [:lsalzman]]

(In reply to Mason Chang [:mchang] from comment #9)
> Comment on attachment 8803014 [details] [diff] [review]
> Bug 1299435 - part 2 - fix Moz2d for Skia m55 update
> 
> Review of attachment 8803014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Factory.cpp
> @@ +383,5 @@
> >        RefPtr<DrawTargetSkia> newTarget;
> >        newTarget = new DrawTargetSkia();
> > +      if (newTarget->Init(aData, aSize, aStride, aFormat, aUninitialized)) {
> > +        retVal = newTarget;
> > +      }
> 
> nit: gfxCriticalError if we can't make the draw target

The code down below that already checks if retVal is null and does the error for us, so that part comes for free.

> ::: gfx/2d/SourceSurfaceSkia.cpp
> @@ +76,5 @@
> > +        aFormat :
> > +        SkiaColorTypeToGfxFormat(pixmap.colorType(), pixmap.alphaType());
> > +    mStride = pixmap.rowBytes();
> > +  } else if (aFormat != SurfaceFormat::UNKNOWN) {
> > +    mFormat = aFormat;
> 
> Should we be worried here if peekPixels actually fail? Why would this happen?

peekPixels will only work on raster images, not GPU ones, so this is expected. I can added a comment. In the GPU case, we want to fall back to reporting the assumed format, since the internal format is both ridiculously difficult to acquire and not necessarily accurate. 

> @@ +131,5 @@
> > +    SkPixmap pixmap;
> > +    if (mImage->peekPixels(&pixmap)) {
> > +      mImage = SkImage::MakeRasterCopy(pixmap);
> > +      if (!mImage) {
> > +        gfxCriticalError() << "Failed copying Skia raster snapshot";
> 
> nit: Should also check if mImage->peekPixels returns false also.

Same as above, this will only be false for the GPU case, which we are handling via copy-on-write. I can comment this.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd4625e101a
part 1 - fix Skia moz.build for Skia m55 update. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ca6acef0a2
part 2 - fix Moz2d for Skia m55 update. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/51ad497c7ac2
part 3 - fix Skia npapi for Skia m55 update. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed742f88f49
part 4 - update Skia source to m55. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab9d3cb13aa
part 5 - adjust fuzz for Skia m55 update. r=mchang

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d587a1cab8f
followup - fix Android fuzz for Skia m55. r=me

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this all out for windows reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38143293&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/e95bbf148d9dcc1b8f753f97f59ec9e55ad258fd

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f997223b1815
part 1 - fix Skia moz.build for Skia m55 update. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/305afd8b3642
part 2 - fix Moz2d for Skia m55 update. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b54f38b6f3
part 3 - fix Skia npapi for Skia m55 update. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/1862062298f7
part 4 - update Skia source to m55. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/a90c5c087244
part 5 - adjust fuzz for Skia m55 update. r=mchang

Comment 15

[Lee Salzman [:lsalzman]]

(In reply to Wes Kocher (:KWierso) from comment #13)
> I had to back this all out for windows reftest failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=38143293&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e95bbf148d9dcc1b8f753f97f59ec9e55ad258fd

There was a bug in shadow blur that snuck in and was only turned up by a newly added reftest. This new push should fix it.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

as a note, we see nice talos svg improvements when this lands:
== Change summary for alert #3805 (as of October 24 2016 19:42 UTC) ==

Improvements:

 27%  tsvgr_opacity summary windows7-32 opt          743.06 -> 544.45
  8%  tsvgx summary windows7-32 opt                  335.14 -> 307.07
  7%  tsvgr_opacity summary windowsxp pgo e10s       387.69 -> 359.69
  6%  tsvgr_opacity summary windows7-32 pgo e10s     389.88 -> 365.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=3805

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f997223b1815
https://hg.mozilla.org/mozilla-central/rev/305afd8b3642
https://hg.mozilla.org/mozilla-central/rev/62b54f38b6f3
https://hg.mozilla.org/mozilla-central/rev/1862062298f7
https://hg.mozilla.org/mozilla-central/rev/a90c5c087244

Comment 18

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Thank you very much! \o/