Closed Bug 755869 Opened 8 years ago Closed 8 years ago

[Skia] Update Skia to a more recent revision

Categories

(Core :: Graphics, enhancement)

x86_64
Linux
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: gw280, Assigned: gw280)

References

(Blocks 1 open bug)

Details

Attachments

(14 files, 10 obsolete files)

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
: 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.
Attached patch fix b2g buildSplinter Review
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.
Blocks: 751418
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?
Attachment #627054 - Flags: review?(mwu) → review+
Attachment #625214 - Attachment is obsolete: true
Attachment #625218 - Attachment is obsolete: true
Attachment #625215 - Attachment is obsolete: true
Attachment #625216 - Attachment is obsolete: true
Attachment #625217 - Attachment is obsolete: true
Attachment #625219 - Attachment is obsolete: true
Attachment #625220 - Attachment is obsolete: true
Attachment #625221 - Attachment is obsolete: true
Attachment #625222 - Attachment is obsolete: true
Attachment #625223 - Attachment is obsolete: true
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+
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.
Here's the patch from bug 719575 on top of the new skia.
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.
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+
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
Attachment #628391 - Flags: review?(joe)
Attachment #628391 - Flags: review?(joe) → review+
(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.
You need to log in before you can comment on or make changes to this bug.