755869 - [Skia] Update Skia to a more recent revision

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[George Wright (:gw280) (needinfo me!)]

We should try to keep up to date. At the moment we're running r2980, and upstream they're on r3817.

Comment 1

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [1/10] Update Skia to r3996

Comment 2

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [2/10] Update Makefile.in for the new Skia

Comment 3

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [3/10] Re-apply bug 689069 - Patch to get arm opts to build with frame pointers enabled.

Comment 4

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [4/10] Re-apply bug 687189 - Implement SkPaint::getPosTextPath

Comment 5

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [5/10] Re-apply bug 688366 - Fix Skia marking radial gradients with the same radius as invalid.

Comment 6

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [6/10] Re-apply SkUserConfig (no original bug)

Comment 7

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [7/10] Re-apply bug 722011 - Fix trailing commas at end of enum lists

Comment 8

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [8/10] Re-apply bug 731384 - Fix compile errors on older versions of clang

Comment 9

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [9/10] Re-apply bug 751814 - Various Skia fixes for ARM without EDSP and ARMv6+

Comment 10

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [10/10] Re-apply bug 719872 - Fix crash on Android by reverting to older FontHost impl

Comment 11

[George Wright (:gw280) (needinfo me!)]

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

Comment 12

[George Wright (:gw280) (needinfo me!)]

Attachment: 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

Comment 13

[George Wright (:gw280) (needinfo me!)]

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.

Comment 14

[Michael Wu [:mwu]]

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?

Comment 15

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [3] Re-apply bug 689069 - Patch to get arm opts to build with frame pointers enabled.

Comment 16

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [4] Re-apply bug 687189 - Implement SkPaint::getPosTextPath

Comment 17

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [5] Re-apply bug 688366 - Fix Skia marking radial gradients with the same radius as invalid.

Comment 18

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [6] Re-apply SkUserConfig (no original bug)

Comment 19

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [7] Re-apply bug 722011 - Fix trailing commas at end of enum lists

Comment 20

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [8] Re-apply bug 731384 - Fix compile errors on older versions of clang

Comment 21

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [9] Re-apply bug 751814 - Various Skia fixes for ARM without EDSP and ARMv6+

Comment 22

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [10] Re-apply bug 719872 - Fix crash on Android by reverting to older FontHost impl

Comment 23

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [11] Re-apply bug 687188 - Skia radial gradients should use the 0/1 color stop values for clamping.

Comment 24

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [12] Re-apply bug 749533 - Add support for GNU/kFreeBSD and Hurd in Skia.

Comment 25

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - [13] Re-apply bug 750733 - Use handles in API object hooks where possible

Comment 26

[George Wright (:gw280) (needinfo me!)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=545eb36e7207

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(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.

Comment 29

[u408661]

It looks like applying the patch from bug 719575 on top of the new skia fixes the clang build errors.

Comment 30

[u408661]

Attachment: fix clang mac build

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

Comment 31

[Boris Zbarsky [:bzbarsky]]

Can we possibly get that into our set of local changes to skia, if we have one, so we don't keep clobbering it?

Comment 32

[George Wright (:gw280) (needinfo me!)]

Yes, I'll update our local patches to skia.

Comment 33

[Ed Morley [:emorley]]

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

Comment 34

[Gregory Szorc [:gps]]

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

Comment 35

[Hubert Figuiere [:hub]]

same here.

Comment 36

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Looks like this isn't the first time. We can probably do exactly what happened last time with bug 719575 (s/SkFloatToScalar/SkScalar/)

Comment 37

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

bah, that was already said... apologies for the bugspam

Comment 38

[George Wright (:gw280) (needinfo me!)]

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.

Comment 39

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/mozilla-central/rev/e8a025a7101b

Comment 41

[Landry Breuil (:gaston)]

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

Comment 42

[George Wright (:gw280) (needinfo me!)]

Attachment: Bug 755869 - Update the patches directory

Review: :joedrew!

Comment 43

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71ebeef47ada

Comment 44

[Ed Morley [:emorley]]

> Bug 755869 - Update the patches directory

https://hg.mozilla.org/mozilla-central/rev/71ebeef47ada

Comment 45

[Matt Brubeck (:mbrubeck)]

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?

Comment 46

[Mike Hommey [:glandium]]

(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.

Comment 47

[Matt Brubeck (:mbrubeck)]

(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.