Update Skia to m55 branch

VERIFIED FIXED in Firefox 52

Status

()

P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: Virtual, Assigned: lsalzman)

Tracking

(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

2 years 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

2 years ago
Blocks: 1309073
(Assignee)

Updated

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

Updated

2 years ago
Depends on: 1309725
(Assignee)

Comment 2

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d587a1cab8f
followup - fix Android fuzz for Skia m55. r=me

Comment 14

2 years 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

2 years 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
Thank you very much! \o/
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Blocks: 1314809

Updated

2 years ago
Duplicate of this bug: 1306890

Updated

2 years ago
Depends on: 1320644
Depends on: 1329296

Updated

2 years ago
See Also: → bug 1334456
(Assignee)

Updated

2 years ago
Blocks: 1347094
Depends on: 1348976
You need to log in before you can comment on or make changes to this bug.