Recent Skia versions do not build on big endian machines anymore

NEW
Unassigned

Status

()

Core
Graphics
P3
normal
3 years ago
9 days ago

People

(Reporter: Martin Husemann, Unassigned)

Tracking

36 Branch
Sun
NetBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8579319 [details] [diff] [review]
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.
Blocks: 1105087
(Reporter)

Comment 1

3 years ago
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!
Well that's the sorry state of exotic archs support.. see https://bugzilla.mozilla.org/show_bug.cgi?id=1005535 for another approach.
(Reporter)

Comment 3

3 years ago
I don't think 1005535 will work on sparc* w/o massive work (which I right now can not do).
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.
Attachment #8579319 - Flags: review?(gwright)
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
Attachment #8579319 - Flags: review?(gwright) → review+
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.
(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.
Flags: needinfo?(landry)
Whiteboard: [gfx-noted]
Comment hidden (obsolete)
(Reporter)

Comment 9

3 years ago
(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.
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.
Flags: needinfo?(landry)
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"
(Reporter)

Comment 12

3 years ago
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.
So you used --disable-skia-gpu i suppose ?
(Reporter)

Comment 14

3 years ago
No, --disable-skia
We're about to make this worse.  See bug 1323303.

Comment 16

9 months ago
--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

9 months ago
(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

Updated

9 months ago
See Also: → bug 1389733
Priority: -- → P3
(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 <stransky@redhat.com> in their current Firefox 59 package which fixes the problem for me (see attached).
Created attachment 8948259 [details] [diff] [review]
Patch by Martin Stransky <stransky@redhat.com> to fix skia on BE
Attachment #8948259 - Flags: review?(martin)
(Reporter)

Comment 20

3 months ago
Comment on attachment 8948259 [details] [diff] [review]
Patch by Martin Stransky <stransky@redhat.com> 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)
Attachment #8948259 - Flags: review?(martin) → review+
(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.
Created attachment 8958424 [details] [diff] [review]
0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch

Here's a cleaned up version of that patch.
Attachment #8948259 - Attachment is obsolete: true
Attachment #8958424 - Flags: review?(milan)
Attachment #8958424 - Flags: review?(martin)
Created attachment 8960052 [details] [diff] [review]
0001-Bug-1144632-gfx-skia-Fix-skia-build-on-Big-Endian-pl.patch

Updated patch.
Attachment #8958424 - Attachment is obsolete: true
Attachment #8958424 - Flags: review?(milan)
Attachment #8958424 - Flags: review?(martin)
Attachment #8960052 - Flags: review?(milan)
Attachment #8960052 - Flags: review?(martin)

Comment 24

2 months ago
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) {
(Reporter)

Comment 25

2 months ago
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.
Attachment #8960052 - Flags: review?(martin) → review+
Attachment #8960052 - Flags: review?(milan) → review?(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.
Attachment #8960052 - Flags: review?(lsalzman) → review-
Created attachment 8967662 [details] [diff] [review]
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.
Attachment #8960052 - Attachment is obsolete: true
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.
Attachment #8967662 - Attachment is obsolete: true

Comment 29

a month ago
I can confirm that with your latest patch I can build Firefox on sparc.
Attachment #8969223 - Flags: review?(lsalzman)
You need to log in before you can comment on or make changes to this bug.