Closed Bug 1144632 Opened 5 years ago Closed 2 years ago

Recent Skia versions do not build on big endian machines anymore

Categories

(Core :: Graphics, defect, P3)

36 Branch
Sun
NetBSD
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: martin, Assigned: lsalzman)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 6 obsolete files)

Sparc64 used Skia for some time, but the 36 branch broke this and now Firefox does not build at all any more.
Blocks: 1105087
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.
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]
(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"
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 ?
No, --disable-skia
We're about to make this worse.  See bug 1323303.
--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
(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
See Also: → 1389733
(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).
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.
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)
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)
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 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-
Attaching patch updated for the current development version.

Should probably try to get this upstreamed to skia.
Attachment #8960052 - Attachment is obsolete: true
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
I can confirm that with your latest patch I can build Firefox on sparc.
Attachment #8969223 - Flags: review?(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?
Flags: needinfo?(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.
Flags: needinfo?(glaubitz)
Cleaned up the earlier versions of this patch to get rid of bit rot.
Assignee: nobody → lsalzman
Attachment #8579319 - Attachment is obsolete: true
Attachment #8969223 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8969223 - Flags: review?(lsalzman)
Attachment #8980162 - Flags: review?(rhunt)
Attachment #8980162 - Flags: review?(rhunt) → review+
https://hg.mozilla.org/mozilla-central/rev/4f26f8702696
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
See Also: → 1469116
You need to log in before you can comment on or make changes to this bug.