1082598 - Update Skia to master revision 53c5d5fb795fe04bec050c0583223027c25b839b (Dec 3, 2015)

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Jeff Muizelaar [:jrmuizel]]

We need this to improve Skia's masking performance. This was showing up on TART during shadow drawing.

https://skia.googlesource.com/skia.git/+/60e4ad7b29f50ebd7698d2d37580d5c8da5ce600

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

We also want something with color glyph support.  Looks like ~March 2015 or later.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: part 0 - update Skia to master revision 53c5d5fb795fe04bec050c0583223027c25b839b

Patch is gzip'd for sanity reasons and attachment size limits. This also includes our Mozilla-specific modifications over the forked revision of Skia.

The github repo containing the forked sources and individual commits for the Mozilla modifications can be found at: https://github.com/lsalzman/moz-skia

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: part 1 - fix Moz2D Skia usage for Skia update

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: part 2 - fix thebes gfxPlatform shutdown for Skia

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: part 3 - workaround for naming conflict in unified sources for Skia and thebes DWrite fonts

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: part 4 - fix Skia NPAPI for Skia update

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: part 5 - fix layers for Skia update

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: part 6 - update moz.build for Skia update

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: part 7 - fix OpenGL interface glue for Skia update

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: part 8 - fix test failures for Skia update

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8695490 [details] [diff] [review]
part 6 - update moz.build for Skia update

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

::: gfx/skia/moz.build
@@ +684,5 @@
>  
>  if CONFIG['_MSC_VER']:
>      # MSVC doesn't need special compiler flags, but Skia needs to be told that these files should
>      # be built with the required SSE level or it will simply compile in stubs and cause runtime crashes
> +    SOURCES['skia/src/opts/SkBitmapFilter_opts_SSE2.cpp'].flags += ['/arch:SSE2 -DSK_CPU_SSE_LEVEL=20']

Why doesn't the previous version have /arch:SSE2?

Comment 12

[Lee Salzman [:lsalzman]]

Comment on attachment 8695494 [details] [diff] [review]
part 8 - fix test failures for Skia update

Regarding the test_canvas.html changes, since the last Skia update, Skia has changed its 0-length stroke pruning behavior such that they will no longer be pruned if there are caps. So the divergence in behavior in that test had to be removed, so at least we're consistent now in not passing that test with other backends.

Skia also recently made some changes to its gradient handling, so there is some required new fuzz in a few gradient-related test cases.

Regarding WPT, we no longer fail in two cases (2d.path.arcTo.shape.curve*) on Windows XP/7 with Skia, so those fails are removed.

The new pruning behavior that caused the test_canvas.html changes also affects a few WPT pruning tests (2d.path.stroke.prune.*), so they now check for Windows XP/7 and are marked as fails on those platforms.

Comment 13

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Comment on attachment 8695490 [details] [diff] [review]
> part 6 - update moz.build for Skia update
> 
> Review of attachment 8695490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/skia/moz.build
> @@ +684,5 @@
> >  
> >  if CONFIG['_MSC_VER']:
> >      # MSVC doesn't need special compiler flags, but Skia needs to be told that these files should
> >      # be built with the required SSE level or it will simply compile in stubs and cause runtime crashes
> > +    SOURCES['skia/src/opts/SkBitmapFilter_opts_SSE2.cpp'].flags += ['/arch:SSE2 -DSK_CPU_SSE_LEVEL=20']
> 
> Why doesn't the previous version have /arch:SSE2?

Before it used to almost exclusively use inline assembly with some occasional intrinsics, so the arch flag wasn't needed. But now Skia has moved towards utilizing some MSVC attributes like "__vectorcall" that require /arch:SSE2 to be used (as in: fail to compile otherwise) in addition to getting rid of the inline assembly in favor of intrinsics.

Comment 14

[Lee Salzman [:lsalzman]]

Giant all-platform try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0937c16da5c1

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3805015270
https://hg.mozilla.org/integration/mozilla-inbound/rev/c239a7bc6cba
https://hg.mozilla.org/integration/mozilla-inbound/rev/3183d30eb549
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8f87df8c68
https://hg.mozilla.org/integration/mozilla-inbound/rev/da976e4cd253
https://hg.mozilla.org/integration/mozilla-inbound/rev/8754fd718869
https://hg.mozilla.org/integration/mozilla-inbound/rev/c22c1bfd091a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4297c82165f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f60e42aca2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/018c2a3031f4

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Fingers crossed :)  Let's change the title of this bug to contain the unique identifier for the version of Skia we picked up, whatever that may be.

Comment 17

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

Backing it all out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e6523d21523f because it turned web-platform-tests(1) permaorange on OSX 10.10:

https://treeherder.mozilla.org/logviewer.html#?job_id=18735556&repo=mozilla-inbound

Comment 18

[Phil Ringnalda (:philor)]

Also Linux e10s reftests (379461-3-container-xhtml.html) and b2g emulator reftest-19 (writing-mode/1090168-1.html).

Comment 19

[Lee Salzman [:lsalzman]]

Attachment: fix test failures for Skia update

Updated the Windows WPT mods to also include 10.10 as they were both affected the same. Relaxed the fuzz introduced by :mchang in bug 1230357 since it behaves slightly differently against updated Skia.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3503094c48d
https://hg.mozilla.org/integration/mozilla-inbound/rev/62701d264788
https://hg.mozilla.org/integration/mozilla-inbound/rev/81f64232e6f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bcb6057f789
https://hg.mozilla.org/integration/mozilla-inbound/rev/09810f69f788
https://hg.mozilla.org/integration/mozilla-inbound/rev/16923db5df37
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8ca41426b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/be17fb6c3c88
https://hg.mozilla.org/integration/mozilla-inbound/rev/1318d121c4ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/6630a176477e

Comment 21

[William Lachance (:wlach)]

Unfortunately this seems to have caused a rather large canvasmark regression. See bug 1233858

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3503094c48d
https://hg.mozilla.org/mozilla-central/rev/62701d264788
https://hg.mozilla.org/mozilla-central/rev/81f64232e6f7
https://hg.mozilla.org/mozilla-central/rev/0bcb6057f789
https://hg.mozilla.org/mozilla-central/rev/09810f69f788
https://hg.mozilla.org/mozilla-central/rev/16923db5df37
https://hg.mozilla.org/mozilla-central/rev/9b8ca41426b7
https://hg.mozilla.org/mozilla-central/rev/be17fb6c3c88
https://hg.mozilla.org/mozilla-central/rev/1318d121c4ff
https://hg.mozilla.org/mozilla-central/rev/6630a176477e

Comment 23

[Landry Breuil (:gaston)]

Saw this build failure on OpenBSD:

/home/othersrc/mozilla-central/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:60:5: error: unknown type name 'locale_t'
    locale_t fOldLocale;
    ^
/home/othersrc/mozilla-central/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:61:5: error: unknown type name 'locale_t'
    locale_t fLocale;

Sorry but locale_t isnt portable everywhere.. guess i'll have to file a followup.

Comment 24

[Lee Salzman [:lsalzman]]

(In reply to Landry Breuil (:gaston) from comment #23)
> Saw this build failure on OpenBSD:
> 
> /home/othersrc/mozilla-central/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:60:
> 5: error: unknown type name 'locale_t'
>     locale_t fOldLocale;
>     ^
> /home/othersrc/mozilla-central/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:61:
> 5: error: unknown type name 'locale_t'
>     locale_t fLocale;
> 
> Sorry but locale_t isnt portable everywhere.. guess i'll have to file a
> followup.

Please file an upstream bug report at: https://bugs.chromium.org/p/skia/issues

Comment 25

[Landry Breuil (:gaston)]

(In reply to Lee Salzman [:lsalzman] from comment #24)
> (In reply to Landry Breuil (:gaston) from comment #23)
> > Saw this build failure on OpenBSD:
> > 
> > /home/othersrc/mozilla-central/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:60:
> > 5: error: unknown type name 'locale_t'
> >     locale_t fOldLocale;
> >     ^
> > /home/othersrc/mozilla-central/gfx/skia/skia/src/gpu/GrAutoLocaleSetter.h:61:
> > 5: error: unknown type name 'locale_t'
> >     locale_t fLocale;
> > 
> > Sorry but locale_t isnt portable everywhere.. guess i'll have to file a
> > followup.
> 
> Please file an upstream bug report at:
> https://bugs.chromium.org/p/skia/issues

Been there, done that, and it's incredibly painful to get things upstreamed in skia, so no thanks. I'll just file a followup here, because i'm only concerned about keeping m-c building on OpenBSD, and this skia update broke it. Life is too short for dealing with that nightmare.