Closed Bug 1299435 Opened 3 years ago Closed 3 years ago

Update Skia to m55 branch

Categories

(Core :: Graphics, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox52 --- verified

People

(Reporter: Virtual, Assigned: lsalzman)

References

(Blocks 1 open bug, )

Details

(Keywords: feature, nightly-community, Whiteboard: [gfx-noted])

Attachments

(5 files)

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.
Whiteboard: [gfx-noted]
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
OS: Unspecified → All
Hardware: Unspecified → All
m55 is now a stable branch since 2016-10-06.
Flags: needinfo?(lsalzman)
Summary: Update Skia to m54 branch → Update Skia to m55 branch
Blocks: 1309073
Assignee: nobody → lsalzman
Severity: major → normal
Status: NEW → ASSIGNED
Flags: needinfo?(lsalzman)
Keywords: feature
Priority: -- → P1
Depends on: 1309725
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.
Attachment #8803013 - Flags: review?(mchang)
Oops, mixup, see comment for part 1 patch for part 2 details. :)
Attachment #8803014 - Flags: review?(mchang)
Tweaking some old NPAPI code that would no longer build due to API changes.
Attachment #8803015 - Flags: review?(mchang)
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.
Attachment #8803016 - Flags: review?(mchang)
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.
Attachment #8803019 - Flags: review?(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
Attachment #8803013 - Flags: review?(mchang) → review+
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?
Attachment #8803016 - Flags: review?(mchang) → review+
Attachment #8803015 - Flags: review?(mchang) → review+
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.
Attachment #8803014 - Flags: review?(mchang) → review+
Attachment #8803019 - Flags: review?(mchang) → review+
(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.
Pushed by lsalzman@mozilla.com:
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
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d587a1cab8f
followup - fix Android fuzz for Skia m55. r=me
Pushed by lsalzman@mozilla.com:
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
(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.
Flags: needinfo?(lsalzman)
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
Blocks: 1314809
Duplicate of this bug: CVE-2017-5406
Depends on: 1320644
Depends on: 1329296
See Also: → 1334456
Blocks: 1347094
Depends on: 1348976
You need to log in before you can comment on or make changes to this bug.