Update Skia to m55 branch

VERIFIED FIXED in Firefox 52

Status

()

Core
Graphics
P1
normal
VERIFIED FIXED
a year ago
11 days ago

People

(Reporter: Virtual, Assigned: lsalzman)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {feature, nightly-community})

unspecified
mozilla52
feature, nightly-community
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [gfx-noted], URL)

Attachments

(5 attachments)

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.

Updated

a year ago
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
(Assignee)

Updated

a year ago
Blocks: 1309073
(Assignee)

Updated

a year ago
Assignee: nobody → lsalzman
Severity: major → normal
Status: NEW → ASSIGNED
Flags: needinfo?(lsalzman)
Keywords: feature
Priority: -- → P1
(Assignee)

Updated

a year ago
Depends on: 1309725
(Assignee)

Comment 2

a year ago
Created attachment 8803013 [details] [diff] [review]
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.
Attachment #8803013 - Flags: review?(mchang)
(Assignee)

Comment 3

a year ago
Created attachment 8803014 [details] [diff] [review]
Bug 1299435 - part 2 - fix Moz2d for Skia m55 update

Oops, mixup, see comment for part 1 patch for part 2 details. :)
Attachment #8803014 - Flags: review?(mchang)
(Assignee)

Comment 4

a year ago
Created attachment 8803015 [details] [diff] [review]
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.
Attachment #8803015 - Flags: review?(mchang)
(Assignee)

Comment 5

a year ago
Created attachment 8803016 [details] [diff] [review]
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.
Attachment #8803016 - Flags: review?(mchang)
(Assignee)

Comment 6

a year ago
Created attachment 8803019 [details]
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.
Attachment #8803019 - Flags: review?(mchang)

Updated

a year ago
Blocks: 1306890
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+
(Assignee)

Comment 10

a year ago
(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

a year ago
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

Comment 12

a year ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d587a1cab8f
followup - fix Android fuzz for Skia m55. r=me
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
Flags: needinfo?(lsalzman)

Comment 14

a year ago
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
(Assignee)

Comment 15

a year ago
(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

Comment 17

a year ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Thank you very much! \o/
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Blocks: 1314809

Updated

11 months ago
Duplicate of this bug: 1306890

Updated

11 months ago
Depends on: 1320644
Depends on: 1329296

Updated

9 months ago
See Also: → bug 1334456
(Assignee)

Updated

7 months ago
Blocks: 1347094
Keywords: nightly-community
QA Contact: Virtual
Depends on: 1348976
You need to log in before you can comment on or make changes to this bug.