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

RESOLVED FIXED in Firefox 46

Status

()

Core
Graphics
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: jrmuizel, Assigned: lsalzman)

Tracking

(Blocks: 5 bugs)

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

3 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

2 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
Blocks: 1209480
(Assignee)

Updated

2 years ago
Blocks: 1221312
(Assignee)

Updated

2 years ago
Blocks: 1221304
(Assignee)

Updated

2 years ago
Blocks: 1221322
(Assignee)

Updated

2 years ago
Blocks: 1221272
(Assignee)

Updated

2 years ago
Blocks: 1142056
(Assignee)

Updated

2 years ago
Blocks: 1228127
(Assignee)

Updated

2 years ago
Blocks: 1228128

Updated

2 years ago
Depends on: 1229918

Updated

2 years ago
Depends on: 1229932

Updated

2 years ago
Blocks: 1229946

Updated

2 years ago
Blocks: 1229972

Updated

2 years ago
Blocks: 1229973

Updated

2 years ago
Blocks: 1229975

Updated

2 years ago
Blocks: 1229977

Updated

2 years ago
Blocks: 1229983

Updated

2 years ago
Blocks: 1230096

Updated

2 years ago
Blocks: 1230098

Updated

2 years ago
Blocks: 1230111
(Assignee)

Comment 2

2 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

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

Comment 3

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

Updated

2 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

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

Comment 5

2 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

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

Updated

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

Comment 7

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

Comment 8

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

Comment 9

2 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

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 11

2 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

2 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

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

Comment 14

2 years ago
Giant all-platform try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0937c16da5c1
(Assignee)

Updated

2 years ago
Blocks: 1230523
(Reporter)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

2 years ago
Attachment #8695477 - Flags: review?(jmuizelaar) → review+

Comment 15

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

2 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

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

Comment 20

2 years ago
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
Depends on: 1233858
Unfortunately this seems to have caused a rather large canvasmark regression. See bug 1233858
Depends on: 1233966

Comment 22

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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

2 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
Depends on: 1234419
(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
Depends on: 1239702

Updated

a year ago
Blocks: 1210383

Updated

a year ago
Blocks: 1106808

Updated

a year ago
Depends on: 1244454
See Also: → bug 1248525
See Also: → bug 1248398

Updated

a year ago
Depends on: 1252275
(Assignee)

Updated

a year ago
No longer blocks: 1229977

Updated

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