[Skia] Update Skia to a more recent revision

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: gw280, Assigned: gw280)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments, 10 obsolete attachments)

889 bytes, patch
mwu
: review+
Details | Diff | Splinter Review
2.07 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.69 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.22 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.53 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
15.94 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.07 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.29 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
21.86 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
10.92 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.07 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.05 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.27 KB, patch
gw280
: review+
Details | Diff | Splinter Review
64.81 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
We should try to keep up to date. At the moment we're running r2980, and upstream they're on r3817.
Created attachment 625214 [details] [diff] [review]
Bug 755869 - [1/10] Update Skia to r3996
Created attachment 625215 [details] [diff] [review]
Bug 755869 - [2/10] Update Makefile.in for the new Skia
Created attachment 625216 [details] [diff] [review]
Bug 755869 - [3/10] Re-apply bug 689069 - Patch to get arm opts to build with frame pointers enabled.
Created attachment 625217 [details] [diff] [review]
Bug 755869 - [4/10] Re-apply bug 687189 - Implement SkPaint::getPosTextPath
Created attachment 625218 [details] [diff] [review]
Bug 755869 - [5/10] Re-apply bug 688366 - Fix Skia marking radial gradients with the same radius as invalid.
Created attachment 625219 [details] [diff] [review]
Bug 755869 - [6/10] Re-apply SkUserConfig (no original bug)
Created attachment 625220 [details] [diff] [review]
Bug 755869 - [7/10] Re-apply bug 722011 - Fix trailing commas at end of enum lists
Created attachment 625221 [details] [diff] [review]
Bug 755869 - [8/10] Re-apply bug 731384 - Fix compile errors on older versions of clang
Created attachment 625222 [details] [diff] [review]
Bug 755869 - [9/10] Re-apply bug 751814 - Various Skia fixes for ARM without EDSP and ARMv6+
Created attachment 625223 [details] [diff] [review]
Bug 755869 - [10/10] Re-apply bug 719872 - Fix crash on Android by reverting to older FontHost impl
https://tbpl.mozilla.org/?tree=Try&rev=d7240a3dffeb
Created attachment 627054 [details] [diff] [review]
fix b2g build

From IRC:

01:49:12 < gw280> cjones: does this look right to you: https://hg.mozilla.org/try/rev/948df947f0aa
01:52:09 < cjones> gw280, maybe, what's it supposed to do?
01:53:47 < gw280> cjones: with the skia rebase they updated SkThread_platform.h to inline different atomic functions depending on whether you're configured to build for Android or Android NDK
01:54:34 < gw280> cjones: some files in libui in gonk are including skia headers and somehow they're getting mismatched between NDK and non-NDK, which causes a link failure with b2g on the skia rebase because symbols are multiply defined as different things 
01:54:54 < gw280> cjones: so this just does what we do in gfx/skia anyway which is say we're always building for the NDK
01:55:30 < cjones> gw280, that seems ok
01:56:03 < cjones> might want to check with mwu
Attachment #627054 - Flags: review?(mwu)
Try builds are:

b2g fix:

https://tbpl.mozilla.org/?tree=Try&rev=948df947f0aa

rebase to r4037 w/azure canvas force enabled:

https://tbpl.mozilla.org/?tree=Try&rev=06aa51a1b0b5

Only the Linux/Android reftests should be considered on these builds as they're the only ones where azure => skia by default.
(Assignee)

Updated

5 years ago
Blocks: 751418

Comment 14

5 years ago
Currently we rely on http://hg.mozilla.org/mozilla-central/file/bd4201e13a9f/gfx/skia/include/core/SkPreConfig.h#l39 to set SK_BUILD_FOR_ANDROID_NDK for us on B2G. Why doesn't this work anymore?

Updated

5 years ago
Attachment #627054 - Flags: review?(mwu) → review+
(Assignee)

Updated

5 years ago
Attachment #625214 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625218 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625215 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625216 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625217 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625219 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625220 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625221 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625222 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625223 - Attachment is obsolete: true
Created attachment 627707 [details] [diff] [review]
Bug 755869 - [3] Re-apply bug 689069 - Patch to get arm opts to build with frame pointers enabled.
Attachment #627707 - Flags: review?(matt.woodrow)
Created attachment 627708 [details] [diff] [review]
Bug 755869 - [4] Re-apply bug 687189 - Implement SkPaint::getPosTextPath
Attachment #627708 - Flags: review?(matt.woodrow)
Created attachment 627709 [details] [diff] [review]
Bug 755869 - [5] Re-apply bug 688366 - Fix Skia marking radial gradients with the same radius as invalid.
Attachment #627709 - Flags: review?(matt.woodrow)
Created attachment 627710 [details] [diff] [review]
Bug 755869 - [6] Re-apply SkUserConfig (no original bug)
Attachment #627710 - Flags: review?(matt.woodrow)
Created attachment 627711 [details] [diff] [review]
Bug 755869 - [7] Re-apply bug 722011 - Fix trailing commas at end of enum lists
Attachment #627711 - Flags: review?(matt.woodrow)
Created attachment 627712 [details] [diff] [review]
Bug 755869 - [8] Re-apply bug 731384 - Fix compile errors on older versions of clang
Attachment #627712 - Flags: review?(matt.woodrow)
Created attachment 627713 [details] [diff] [review]
Bug 755869 - [9] Re-apply bug 751814 - Various Skia fixes for ARM without EDSP and ARMv6+
Attachment #627713 - Flags: review?(matt.woodrow)
Created attachment 627714 [details] [diff] [review]
Bug 755869 - [10] Re-apply bug 719872 - Fix crash on Android by reverting to older FontHost impl
Attachment #627714 - Flags: review?(matt.woodrow)
Created attachment 627715 [details] [diff] [review]
Bug 755869 - [11] Re-apply bug 687188 - Skia radial gradients should use the 0/1 color stop values for clamping.
Attachment #627715 - Flags: review?(matt.woodrow)
Created attachment 627716 [details] [diff] [review]
Bug 755869 - [12] Re-apply bug 749533 - Add support for GNU/kFreeBSD and Hurd in Skia.
Attachment #627716 - Flags: review?(matt.woodrow)
Created attachment 627718 [details] [diff] [review]
Bug 755869 - [13] Re-apply bug 750733 - Use handles in API object hooks where possible
Attachment #627718 - Flags: review?(matt.woodrow)
Attachment #627707 - Flags: review?(matt.woodrow) → review+
Attachment #627708 - Flags: review?(matt.woodrow) → review+
Attachment #627709 - Flags: review?(matt.woodrow) → review+
Attachment #627710 - Flags: review?(matt.woodrow) → review+
Attachment #627711 - Flags: review?(matt.woodrow) → review+
Attachment #627712 - Flags: review?(matt.woodrow) → review+
Attachment #627713 - Flags: review?(matt.woodrow) → review+
Attachment #627718 - Flags: review?(matt.woodrow) → review+
Attachment #627716 - Flags: review?(matt.woodrow) → review+
Attachment #627715 - Flags: review?(matt.woodrow) → review+
Attachment #627714 - Flags: review?(matt.woodrow) → review+
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=545eb36e7207

Comment 27

5 years ago
This broke clang builds with this build error (presumably due to -Werror silliness):

../../../mozilla/gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:810:28: error: non-constant-expression cannot be narrowed from type 'CGFloat' (aka 'double') to 'SkScalar' (aka 'float') in initializer list [-Wc++11-narrowing]
    const SkPoint trans = {(vertOffset.width),
                           ^~~~~~~~~~~~~~~~~~
../../../mozilla/gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:810:28: note: override this message by inserting an explicit cast
    const SkPoint trans = {(vertOffset.width),
                           ^~~~~~~~~~~~~~~~~~
                           static_cast<SkScal)r>(

and similar on line 811.
(In reply to Boris Zbarsky (:bz) from comment #27)
> This broke clang builds with this build error (presumably due to -Werror
> silliness):
> 
> ../../../mozilla/gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:810:28:
> error: non-constant-expression cannot be narrowed from type 'CGFloat' (aka
> 'double') to 'SkScalar' (aka 'float') in initializer list [-Wc++11-narrowing]
>     const SkPoint trans = {(vertOffset.width),
>                            ^~~~~~~~~~~~~~~~~~
> ../../../mozilla/gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:810:28:
> note: override this message by inserting an explicit cast
>     const SkPoint trans = {(vertOffset.width),
>                            ^~~~~~~~~~~~~~~~~~
>                            static_cast<SkScal)r>(
> 
> and similar on line 811.

This would be an hard error on c++11, so it is probably better to fix it instead of just disabling the warning.
It looks like applying the patch from bug 719575 on top of the new skia fixes the clang build errors.
Created attachment 627984 [details] [diff] [review]
fix clang mac build

Here's the patch from bug 719575 on top of the new skia.

Comment 31

5 years ago
Can we possibly get that into our set of local changes to skia, if we have one, so we don't keep clobbering it?
Yes, I'll update our local patches to skia.
https://hg.mozilla.org/mozilla-central/rev/5fb409f61b93
https://hg.mozilla.org/mozilla-central/rev/477b8a5a7169
https://hg.mozilla.org/mozilla-central/rev/c3f8b85b916b
https://hg.mozilla.org/mozilla-central/rev/b4eef6c71672
https://hg.mozilla.org/mozilla-central/rev/73e33e3e48cc
https://hg.mozilla.org/mozilla-central/rev/66e29180f635
https://hg.mozilla.org/mozilla-central/rev/04f6554beaa3
https://hg.mozilla.org/mozilla-central/rev/0912e895675b
https://hg.mozilla.org/mozilla-central/rev/304e51f21f0d
https://hg.mozilla.org/mozilla-central/rev/858fb563db58
https://hg.mozilla.org/mozilla-central/rev/544c28fd1629
https://hg.mozilla.org/mozilla-central/rev/4b93ab161451
https://hg.mozilla.org/mozilla-central/rev/1f078aeb5213
https://hg.mozilla.org/mozilla-central/rev/545eb36e7207
Assignee: nobody → gwright
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
This has broken my trunk Clang builds. I'll let others decide how to triage.

SkFontHost_mac_coretext.cpp
../../../gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:810:28: error: non-constant-expression cannot be narrowed from type 'CGFloat' (aka 'double') to 'SkScalar' (aka 'float') in initializer list [-Wc++11-narrowing]
    const SkPoint trans = {SkFloatToScalar(vertOffset.width),
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../gfx/skia/include/core/SkScalar.h:111:37: note: expanded from macro 'SkFloatToScalar'
    #define SkFloatToScalar(n)      (n)
                                    ^~~
../../../gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:810:28: note: override this message by inserting an explicit cast
    const SkPoint trans = {SkFloatToScalar(vertOffset.width),
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                           static_cast<SkScalar>(           )
../../../gfx/skia/include/core/SkScalar.h:111:37: note: expanded from macro 'SkFloatToScalar'
    #define SkFloatToScalar(n)      (n)
                                    ^~~
../../../gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:811:28: error: non-constant-expression cannot be narrowed from type 'CGFloat' (aka 'double') to 'SkScalar' (aka 'float') in initializer list [-Wc++11-narrowing]
                           SkFloatToScalar(vertOffset.height)};
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../gfx/skia/include/core/SkScalar.h:111:37: note: expanded from macro 'SkFloatToScalar'
    #define SkFloatToScalar(n)      (n)
                                    ^~~
../../../gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:811:28: note: override this message by inserting an explicit cast
                           SkFloatToScalar(vertOffset.height)};
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                           static_cast<SkScalar>(            )
../../../gfx/skia/include/core/SkScalar.h:111:37: note: expanded from macro 'SkFloatToScalar'
    #define SkFloatToScalar(n)      (n)
                                    ^~~
../../../gfx/skia/src/ports/SkFontHost_mac_coretext.cpp:1223:16: warning: unused variable 'isA8' [-Wunused-variable]
    const bool isA8 = !isLCD && !isBW;
               ^
1 warning and 2 errors generated.

In the directory  /Users/gps/src/services-central/obj-ff-dbg/gfx/skia
The following command failed to execute properly:
/usr/local/bin/ccache /usr/local/llvm/bin/clang++ -o SkFontHost_mac_coretext.o -c -fvisibility=hidden -DSK_A32_SHIFT=24 -DSK_R32_SHIFT=16 -DSK_G32_SHIFT=8 -DSK_B32_SHIFT=0 -DUSE_SKIA -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -I/Users/gps/src/services-central/gfx/skia/include/core -I/Users/gps/src/services-central/gfx/skia/include/config -I/Users/gps/src/services-central/gfx/skia/include/ports -I/Users/gps/src/services-central/gfx/skia/src/core -I/Users/gps/src/services-central/gfx/skia/include/images -I/Users/gps/src/services-central/gfx/skia/include/utils -I/Users/gps/src/services-central/gfx/skia/include/utils/mac -I/Users/gps/src/services-central/gfx/skia/include/utils/win -I/Users/gps/src/services-central/gfx/skia/include/views -I/Users/gps/src/services-central/gfx/skia/include/effects -I/Users/gps/src/services-central/gfx/skia -I. -I../../dist/include -I../../dist/include/nsprpub -I/Users/gps/src/services-central/obj-ff-dbg/dist/include/nspr -I/Users/gps/src/services-central/obj-ff-dbg/dist/include/nss -fPIC -Qunused-arguments -fno-rtti -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -fcolor-diagnostics -isysroot /Applications/Xcode.app//Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.7.sdk -fno-exceptions -fno-strict-aliasing -std=gnu++0x -ffunction-sections -fdata-sections -pthread -DNO_X11 -pipe -DDEBUG -D_DEBUG -DTRACING -g -O3 -fno-omit-frame-pointer -Qunused-arguments -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/SkFontHost_mac_coretext.pp /Users/gps/src/services-central/gfx/skia/src/ports/SkFontHost_mac_coretext.cpp
make[6]: *** [SkFontHost_mac_coretext.o] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs_tier_platform] Error 2
make[3]: *** [tier_platform] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2
same here.
Looks like this isn't the first time. We can probably do exactly what happened last time with bug 719575 (s/SkFloatToScalar/SkScalar/)
bah, that was already said... apologies for the bugspam
Comment on attachment 627984 [details] [diff] [review]
fix clang mac build

This looks good to me, so I'll land it to fix the build.
Attachment #627984 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e8a025a7101b
Duplicate of this bug: 759411
That skia update broke my builds on OpenBSD/gcc 4.2:

skia/src/core/SkMatrix.cpp:1723: error: extra ';'

The changeset only adds the ; in that file... sigh. If i guess it right, it should be yet another local patch to 'fix' it ?

Dunno what upstream had in their minds when adding it in one of theses changes: https://code.google.com/p/skia/source/list?path=/trunk/src/core/SkMatrix.cpp&start=2246
Blocks: 759671
Blocks: 759683
Created attachment 628391 [details] [diff] [review]
Bug 755869 - Update the patches directory

Review: :joedrew!
(Assignee)

Updated

5 years ago
Attachment #628391 - Flags: review?(joe)
Attachment #628391 - Flags: review?(joe) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/71ebeef47ada
> Bug 755869 - Update the patches directory

https://hg.mozilla.org/mozilla-central/rev/71ebeef47ada
This change added 3 static initializers:
http://graphs.mozilla.org/graph.html#tests=[[81,63,6]]&sel=1338167620593.957,1338331013144.1003&displayrange=30&datatype=running

Should we file a bug against upstream Skia?
(In reply to Matt Brubeck (:mbrubeck) from comment #45)
> This change added 3 static initializers:
> http://graphs.mozilla.org/graph.html#tests=[[81,63,6]]&sel=1338167620593.957,
> 1338331013144.1003&displayrange=30&datatype=running
> 
> Should we file a bug against upstream Skia?

Depends if these patches have been upstreamed.
(In reply to Mike Hommey [:glandium] from comment #46)
> Depends if these patches have been upstreamed.

Most of the changes from this bug came *from* upstream:
https://hg.mozilla.org/mozilla-central/rev/5fb409f61b93

The other patches were mostly re-applying changes that had already been applied to the previous version of skia in mozilla-central.
Blocks: 1210886
You need to log in before you can comment on or make changes to this bug.