Closed Bug 1082598 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Graphics, enhancement)

x86
All
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jrmuizel, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 1 obsolete file)

3.04 MB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
13.43 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1002 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.56 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.59 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.14 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
51.40 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
7.57 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
12.89 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
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
Blocks: skia-updates
We also want something with color glyph support.  Looks like ~March 2015 or later.
OS: Mac OS X → All
Assignee: nobody → lsalzman
Summary: Update Skia to something including "Improve SkARGB32_A8_BlitMask_SSE2" → Update Skia to March 2015 or newer
Blocks: 1221312
Blocks: 1221304
Blocks: 1221322
Blocks: 1221272
Blocks: 1142056
Blocks: 1228127
Blocks: 1228128
Depends on: 1229918
Depends on: 1229932
Blocks: 1229946
Blocks: 1229972
Blocks: 1229973
Blocks: 1229975
Blocks: 1229977
Blocks: 1229983
Blocks: 1230096
Blocks: 1230098
Blocks: 1230111
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
Attachment #8695477 - Flags: review?(jmuizelaar)
Attachment #8695477 - Attachment is patch: true
Attachment #8695477 - Attachment mime type: application/gzip → text/plain
Attachment #8695483 - Flags: review?(jmuizelaar)
Attachment #8695483 - Attachment description: fix Moz2D Skia usage for Skia update → part 1 - fix Moz2D Skia usage for Skia update
Attachment #8695487 - Flags: review?(jmuizelaar)
Attachment #8695487 - Attachment description: fix Skia NPAPI for Skia update → part 4 - fix Skia NPAPI for Skia update
Attachment #8695488 - Flags: review?(jmuizelaar)
Attachment #8695490 - Flags: review?(jmuizelaar)
Attachment #8695494 - Flags: review?(jmuizelaar)
Attachment #8695494 - Attachment description: fix test failures for Skia update → part 8 - fix test failures for Skia update
Attachment #8695487 - Flags: review?(jmuizelaar) → review+
Attachment #8695488 - Flags: review?(jmuizelaar) → review+
Attachment #8695492 - Flags: review?(jmuizelaar) → review+
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?
Attachment #8695490 - Flags: review?(jmuizelaar) → review+
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.
(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.
Blocks: 1230523
Attachment #8695483 - Flags: review?(jmuizelaar) → review+
Attachment #8695485 - Flags: review?(jmuizelaar) → review+
Attachment #8695484 - Flags: review?(jmuizelaar) → review+
Attachment #8695494 - Flags: review?(jmuizelaar) → review+
Attachment #8695477 - Flags: review?(jmuizelaar) → review+
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.
Summary: Update Skia to March 2015 or newer → Update Skia to master revision 53c5d5fb795fe04bec050c0583223027c25b839b (Dec 3, 2015)
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
Flags: needinfo?(lsalzman)
Also Linux e10s reftests (379461-3-container-xhtml.html) and b2g emulator reftest-19 (writing-mode/1090168-1.html).
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.
Attachment #8695494 - Attachment is obsolete: true
Flags: needinfo?(lsalzman)
Attachment #8700088 - Flags: review+
Unfortunately this seems to have caused a rather large canvasmark regression. See bug 1233858
Depends on: 1233966
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.
(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
(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.
Depends on: 1234494
Depends on: 1238795
Blocks: 1210383
Blocks: 1106808
Depends on: 1244454
See Also: → 1248525
See Also: → 1248398
Depends on: 1252275
No longer blocks: 1229977
Blocks: 1249875
Depends on: 1276161
You need to log in before you can comment on or make changes to this bug.