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

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: lsalzman)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

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
(Reporter)

Description

4 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1210886
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
(Assignee)

Updated

3 years ago
Blocks: 1221312
(Assignee)

Updated

3 years ago
Blocks: 1221304
(Assignee)

Updated

3 years ago
Blocks: 1221322
(Assignee)

Updated

3 years ago
Blocks: 1221272
(Assignee)

Updated

3 years ago
Blocks: 1142056
(Assignee)

Updated

3 years ago
Blocks: 1228127
(Assignee)

Updated

3 years ago
Blocks: 1228128

Updated

3 years ago
Depends on: 1229918

Updated

3 years ago
Depends on: 1229932

Updated

3 years ago
Blocks: 1229946

Updated

3 years ago
Blocks: 1229972

Updated

3 years ago
Blocks: 1229973

Updated

3 years ago
Blocks: 1229975

Updated

3 years ago
Blocks: 1229977

Updated

3 years ago
Blocks: 1229983

Updated

3 years ago
Blocks: 1230096

Updated

3 years ago
Blocks: 1230098

Updated

3 years ago
Blocks: 1230111
(Assignee)

Comment 2

3 years ago
Created attachment 8695477 [details] [diff] [review]
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
Attachment #8695477 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8695477 - Attachment is patch: true
Attachment #8695477 - Attachment mime type: application/gzip → text/plain
(Assignee)

Comment 3

3 years ago
Created attachment 8695483 [details] [diff] [review]
part 1 - fix Moz2D Skia usage for Skia update
Attachment #8695483 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8695483 - Attachment description: fix Moz2D Skia usage for Skia update → part 1 - fix Moz2D Skia usage for Skia update
(Assignee)

Comment 4

3 years ago
Created attachment 8695484 [details] [diff] [review]
part 2 - fix thebes gfxPlatform shutdown for Skia
Attachment #8695484 - Flags: review?(jmuizelaar)
(Assignee)

Comment 5

3 years ago
Created attachment 8695485 [details] [diff] [review]
part 3 - workaround for naming conflict in unified sources for Skia and thebes DWrite fonts
Attachment #8695485 - Flags: review?(jmuizelaar)
(Assignee)

Comment 6

3 years ago
Created attachment 8695487 [details] [diff] [review]
part 4 - fix Skia NPAPI for Skia update
Attachment #8695487 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8695487 - Attachment description: fix Skia NPAPI for Skia update → part 4 - fix Skia NPAPI for Skia update
(Assignee)

Comment 7

3 years ago
Created attachment 8695488 [details] [diff] [review]
part 5 - fix layers for Skia update
Attachment #8695488 - Flags: review?(jmuizelaar)
(Assignee)

Comment 8

3 years ago
Created attachment 8695490 [details] [diff] [review]
part 6 - update moz.build for Skia update
Attachment #8695490 - Flags: review?(jmuizelaar)
(Assignee)

Comment 9

3 years ago
Created attachment 8695492 [details] [diff] [review]
part 7 - fix OpenGL interface glue for Skia update
Attachment #8695492 - Flags: review?(jmuizelaar)
(Assignee)

Comment 10

3 years ago
Created attachment 8695494 [details] [diff] [review]
part 8 - fix test failures for Skia update
Attachment #8695494 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8695494 - Attachment description: fix test failures for Skia update → part 8 - fix test failures for Skia update
(Reporter)

Updated

3 years ago
Attachment #8695487 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

3 years ago
Attachment #8695488 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

3 years ago
Attachment #8695492 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 11

3 years ago
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+
(Assignee)

Comment 12

3 years ago
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.
(Assignee)

Comment 13

3 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 1230523
(Reporter)

Updated

3 years ago
Attachment #8695483 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

3 years ago
Attachment #8695485 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

3 years ago
Attachment #8695484 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

3 years ago
Attachment #8695494 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

3 years ago
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.
(Assignee)

Updated

3 years ago
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).
(Assignee)

Comment 19

3 years ago
Created attachment 8700088 [details] [diff] [review]
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.
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

Updated

3 years ago
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.
(Assignee)

Comment 24

3 years ago
(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

Updated

3 years ago
Blocks: 1210383

Updated

3 years ago
Blocks: 1106808

Updated

3 years ago
Depends on: 1244454
See Also: → bug 1248525
See Also: → bug 1248398

Updated

3 years ago
Depends on: 1252275
(Assignee)

Updated

3 years ago
No longer blocks: 1229977

Updated

3 years ago
Blocks: 1249875
Depends on: 1276161
You need to log in before you can comment on or make changes to this bug.