1144632 - Recent Skia versions do not build on big endian machines anymore

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Martin Husemann]

Attachment: Fix configure to enable Skia and make it compile on big endian machines again

Sparc64 used Skia for some time, but the 36 branch broke this and now Firefox does not build at all any more.

Comment 1

[Martin Husemann]

Turns out it fails later without any sane hope of fixing.

F*ck you Skia, what a pile of shite!

Can we back the update all out? The previous version DID work just fine!

Comment 2

[Landry Breuil (:gaston)]

Well that's the sorry state of exotic archs support.. see https://bugzilla.mozilla.org/show_bug.cgi?id=1005535 for another approach.

Comment 3

[Martin Husemann]

I don't think 1005535 will work on sparc* w/o massive work (which I right now can not do).

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8579319 [details] [diff] [review]
Fix configure to enable Skia and make it compile on big endian machines again

Flagging George for review.

Comment 5

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

Comment on attachment 8579319 [details] [diff] [review]
Fix configure to enable Skia and make it compile on big endian machines again

Review of attachment 8579319 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/skia/trunk/include/config/SkUserConfig.h
@@ +201,5 @@
>  #define SK_A32_SHIFT 24
>  #define SK_R32_SHIFT 16
>  #define SK_G32_SHIFT 8
>  #define SK_B32_SHIFT 0
> +#endif

Please set these in SkUserConfig.h

Comment 6

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

Sorry, I misread the patch and thought that you were #if 0ing out codechunks from SkPostConfig.h. Anyway, I can't remember why we limit the skia colour byteorder to little endian only, but I'm ok with removing that codeblock entirely from SkUserConfig.h (no #if 0 please).

As for enabling it everywhere, I'm ok with that in principle, but you'll need to check with some of our other platform maintainers like Landry to see if unconditionally enabling Skia everywhere is going to impact them. It won't impact Mozilla as all our supported platforms support Skia.

Comment 7

[Botond Ballo [:botond]]

(In reply to George Wright (:gw280) from comment #6)
> As for enabling it everywhere, I'm ok with that in principle, but you'll
> need to check with some of our other platform maintainers like Landry to see
> if unconditionally enabling Skia everywhere is going to impact them. It
> won't impact Mozilla as all our supported platforms support Skia.

ni Landry for this.

Comment 8

[Martin Husemann]

Sorry, but making skia work again on BE architectures without GPU support seems to be a bit harder, and we ended up making NetBSD/sparc64 us

Comment 9

[Martin Husemann]

(In reply to Martin Husemann from comment #8)
What I was trying to say:

Sorry, but making skia work again on BE architectures without GPU support seems to be a bit harder.

We ended up making NetBSD/sparc64 work again on the 36 branch by applying patches from bug 1105087, bug 1111395, and bug 1145014.

Will revisit this when we have a chance to actually make GPU acceleration work, but that is blocked by non-firefox related issues currently.

Comment 10

[Landry Breuil (:gaston)]

I wouldnt consider myself a "platform maintainer", im just trying to ensure things keep building on exotic archs. Which is not the case anyway, and even if we pile build hacks all around, runtime is broken in xpcshell during make package, so i think i'll just care less and less over time.

Comment 11

[Landry Breuil (:gaston)]

Fwiw, the build is still broken in skia on powerpc, even with the patch from that bug:

src/mozilla-central/gfx/skia/trunk/include/gpu/GrTypes.h:313:6: error: #error "Skia gpu currently assume
s little endian"

Comment 12

[Martin Husemann]

Sorry if I wasn't clear - I couldn't get this to fully work (see comment 9) and avoided skia in the end. On NetBSD/sparc64 we have no chance to get GPU acceleration to work right now (unrelated to Skia or Firefox), so this did not seem a route worth to spend time on for now.

Comment 13

[Landry Breuil (:gaston)]

So you used --disable-skia-gpu i suppose ?

Comment 14

[Martin Husemann]

No, --disable-skia

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

We're about to make this worse.  See bug 1323303.

Comment 16

[Jan Beich]

--disable-skia is trivial (before bug 1371689) to restore downstream[1] and can be debugged via x86. Given Firefox is in no rush to drop #ifdef USE_CAIRO the rendering regressions will probably be gradual. However, Quantum Render may phase out some Skia/Cairo usage.

[1] https://github.com/freebsd/freebsd-ports/commit/9ad66e6c36f5

Comment 17

[Jan Beich]

(In reply to Jan Beich from comment #16)
> --disable-skia is trivial (before bug 1371689) to restore downstream[1]

Scratch "(before ...)", it only required an extra #ifdef.

[1] FF56 version: https://github.com/mozilla/gecko-dev/commit/3ede77dd2bbc

Comment 18

[John Paul Adrian Glaubitz]

(In reply to Martin Husemann from comment #1)
> Turns out it fails later without any sane hope of fixing.

Fedora upstream ships the patch "build-big-endian.patch" by Martin Stransky <[e-mail]> in their current Firefox 59 package which fixes the problem for me (see attached).

Comment 19

[John Paul Adrian Glaubitz]

Attachment: Patch by Martin Stransky <[e-mail]> to fix skia on BE

Comment 20

[Martin Husemann]

Comment on attachment 8948259 [details] [diff] [review]
Patch by Martin Stransky <[e-mail]> to fix skia on BE

Review of attachment 8948259 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me (but I'm not in a position to actually test it right now, I trust you on that)

Comment 21

[John Paul Adrian Glaubitz]

(In reply to Martin Husemann from comment #20)
> Looks good to me (but I'm not in a position to actually test it right now, I
> trust you on that)

Well, it's not a super-clean patch - it contains commented out code - so I really wouldn't want to merge it as is.

My intention with posting the patch was to get input whether the patch itself would be acceptable in general.
I have verified it to work on both sparc64 and ppc64 (big endian).

If the patch is acceptable in general, I'd be happy to provide a cleaned up version.

Comment 22

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch

Here's a cleaned up version of that patch.

Comment 23

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch

Updated patch.

Comment 24

[Petr Sumbera]

With your patch on Solaris sparc I'm still getting:

37:01.44 /usr/bin/g++ -std=gnu++14 -o ConvolutionFilter.o -c -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/stl_wrappers -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/system_wrappers -include /scratch/firefox/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_SOLARIS=1 -DUSE_CAIRO -DMOZ2D_HAS_MOZ_CAIRO -DMOZ_ENABLE_FREETYPE -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/scratch/firefox/gfx/2d -I/scratch/firefox/obj-sparc64-sun-solaris2.11/gfx/2d -I/scratch/firefox/obj-sparc64-sun-solaris2.11/ipc/ipdl/_ipdlheaders -I/scratch/firefox/ipc/chromium/src -I/scratch/firefox/ipc/glue -I/scratch/firefox/gfx/skia -I/scratch/firefox/gfx/skia/skia/include/config -I/scratch/firefox/gfx/skia/skia/include/core -I/scratch/firefox/gfx/skia/skia/include/gpu -I/scratch/firefox/gfx/skia/skia/include/utils -I/scratch/firefox/gfx/skia/skia/include/private -I/scratch/firefox/gfx/skia/skia/src/core -I/scratch/firefox/gfx/skia/skia/src/image -I/scratch/firefox/gfx/skia/skia/src/gpu -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/include -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/include/nspr -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /scratch/firefox/obj-sparc64-sun-solaris2.11/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe -g -O -fno-omit-frame-pointer -Wno-error=shadow -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/include/cairo -I/usr/include/freetype2 -I/usr/include/libpng14 -I/usr/include/freetype2 -I/usr/include/libpng14  -MD -MP -MF .deps/ConvolutionFilter.o.pp   /scratch/firefox/gfx/2d/ConvolutionFilter.cpp
37:01.85 In file included from /scratch/firefox/gfx/skia/skia/src/core/SkPM4f.h:11:0,
37:01.85                  from /scratch/firefox/gfx/skia/skia/src/core/SkRasterPipeline.h:14,
37:01.85                  from /scratch/firefox/gfx/skia/skia/src/core/SkOpts.h:12,
37:01.85                  from /scratch/firefox/gfx/2d/ConvolutionFilter.cpp:11:
37:01.85 /scratch/firefox/gfx/skia/skia/src/core/SkColorData.h:85:10: error: #error "need 32bit packing to be either RGBA or BGRA"
37:01.85          #error "need 32bit packing to be either RGBA or BGRA"
37:01.85           ^

I don't have working Firefox on sparc. But so far I was using for building following:

diff -r 388d81ed93fa gfx/skia/skia/include/config/SkUserConfig.h
--- a/gfx/skia/skia/include/config/SkUserConfig.h       Tue Jul 25 15:58:16 2017 -0400
+++ b/gfx/skia/skia/include/config/SkUserConfig.h       Thu Aug 10 13:43:23 2017 +0000
@@ -136,10 +136,17 @@
 //#define SK_HISTOGRAM_ENUMERATION(name, value, boundary_value)

 // On all platforms we have this byte order
+#if defined(SK_CPU_BENDIAN)
+#define SK_A32_SHIFT 0
+#define SK_R32_SHIFT 8
+#define SK_G32_SHIFT 16
+#define SK_B32_SHIFT 24
+#else
 #define SK_A32_SHIFT 24
 #define SK_R32_SHIFT 16
 #define SK_G32_SHIFT 8
 #define SK_B32_SHIFT 0
+#endif

 #define SK_ALLOW_STATIC_GLOBAL_INITIALIZERS 0
diff -r 388d81ed93fa gfx/skia/skia/include/gpu/GrTypes.h
--- a/gfx/skia/skia/include/gpu/GrTypes.h       Tue Jul 25 15:58:16 2017 -0400
+++ b/gfx/skia/skia/include/gpu/GrTypes.h       Thu Aug 10 13:43:23 2017 +0000
@@ -326,9 +326,9 @@
 static const int kGrPixelConfigCnt = kLast_GrPixelConfig + 1;

 // Aliases for pixel configs that match skia's byte order.
-#ifndef SK_CPU_LENDIAN
-    #error "Skia gpu currently assumes little endian"
-#endif
+// #ifndef SK_CPU_LENDIAN
+//    #error "Skia gpu currently assumes little endian"
+// #endif
 #if SK_PMCOLOR_BYTE_ORDER(B,G,R,A)
     static const GrPixelConfig kSkia8888_GrPixelConfig = kBGRA_8888_GrPixelConfig;
 #elif SK_PMCOLOR_BYTE_ORDER(R,G,B,A)
diff -r 388d81ed93fa gfx/skia/skia/src/opts/Sk4px_none.h
--- a/gfx/skia/skia/src/opts/Sk4px_none.h       Tue Jul 25 15:58:16 2017 -0400
+++ b/gfx/skia/skia/src/opts/Sk4px_none.h       Thu Aug 10 13:43:23 2017 +0000
@@ -69,7 +69,7 @@
 }

 inline Sk4px Sk4px::alphas() const {
-    static_assert(SK_A32_SHIFT == 24, "This method assumes little-endian.");
+//    static_assert(SK_A32_SHIFT == 24, "This method assumes little-endian.");
     return Sk16b((*this)[ 3], (*this)[ 3], (*this)[ 3], (*this)[ 3],
                  (*this)[ 7], (*this)[ 7], (*this)[ 7], (*this)[ 7],
                  (*this)[11], (*this)[11], (*this)[11], (*this)[11],
@@ -91,7 +91,7 @@
 }

 inline Sk4px Sk4px::zeroAlphas() const {
-    static_assert(SK_A32_SHIFT == 24, "This method assumes little-endian.");
+//    static_assert(SK_A32_SHIFT == 24, "This method assumes little-endian.");
     return Sk16b((*this)[ 0], (*this)[ 1], (*this)[ 2], 0,
                  (*this)[ 4], (*this)[ 5], (*this)[ 6], 0,
                  (*this)[ 8], (*this)[ 9], (*this)[10], 0,
@@ -99,7 +99,7 @@
 }

 inline Sk4px Sk4px::zeroColors() const {
-    static_assert(SK_A32_SHIFT == 24, "This method assumes little-endian.");
+//    static_assert(SK_A32_SHIFT == 24, "This method assumes little-endian.");
     return Sk16b(0,0,0, (*this)[ 3],
                  0,0,0, (*this)[ 7],
                  0,0,0, (*this)[11],
diff -r 388d81ed93fa gfx/skia/skia/src/opts/SkXfermode_opts.h
--- a/gfx/skia/skia/src/opts/SkXfermode_opts.h  Tue Jul 25 15:58:16 2017 -0400
+++ b/gfx/skia/skia/src/opts/SkXfermode_opts.h  Thu Aug 10 13:43:23 2017 +0000
@@ -118,7 +118,7 @@
     inline Sk4f Xfermode::operator()(const Sk4f& d, const Sk4f& s) const

 static inline Sk4f a_rgb(const Sk4f& a, const Sk4f& rgb) {
-    static_assert(SK_A32_SHIFT == 24, "");
+//    static_assert(SK_A32_SHIFT == 24, "");
     return a * Sk4f(0,0,0,1) + rgb * Sk4f(1,1,1,0);
 }
 static inline Sk4f alphas(const Sk4f& f) {

Comment 25

[Martin Husemann]

Comment on attachment 8960052 [details] [diff] [review]
0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch

Review of attachment 8960052 [details] [diff] [review]:
-----------------------------------------------------------------

I can't (even build-) test easily right now, but looks good to me.

Comment 26

[Lee Salzman [:lsalzman]]

Comment on attachment 8960052 [details] [diff] [review]
0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch

This patch does some things that are questionable that I would need to investigate in detail and talk with Martin Stransky to discuss what really should be going on along with the best way to achieve it. For now I am going to shoot down this patch until that is done.

Comment 27

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch

Attaching patch updated for the current development version.

Should probably try to get this upstreamed to skia.

Comment 28

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch

For some reason, the last patch uploaded was still the old one.

This time, the correct one should be attached which works for me.

Comment 29

[Petr Sumbera]

I can confirm that with your latest patch I can build Firefox on sparc.

Comment 30

[Lee Salzman [:lsalzman]]

(In reply to John Paul Adrian Glaubitz from comment #28)
> Created attachment 8969223 [details] [diff] [review]
> 0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch
> 
> For some reason, the last patch uploaded was still the old one.
> 
> This time, the correct one should be attached which works for me.

While there are reports that this patch builds, I am concerned that nobody is actually reporting if the resulting build actually rasterizes things correctly at all and can reasonably browse without odd visual artifacts or related crashes. Can you verify this?

Comment 31

[John Paul Adrian Glaubitz]

(In reply to Lee Salzman [:lsalzman] from comment #30)
> While there are reports that this patch builds, I am concerned that nobody
> is actually reporting if the resulting build actually rasterizes things
> correctly at all and can reasonably browse without odd visual artifacts or
> related crashes. Can you verify this?

Yes, my tests showed no problems in this regard on linux-sparc64. I can perform additional tests on linux-ppc64 (big endian) if desired.

Comment 32

[Lee Salzman [:lsalzman]]

Attachment: fix big-endian Skia builds

Cleaned up the earlier versions of this patch to get rid of bit rot.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f26f8702696
fix big-endian Skia builds. r=rhunt

Comment 34

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4f26f8702696