Closed
Bug 1144632
Opened 10 years ago
Closed 6 years ago
Recent Skia versions do not build on big endian machines anymore
Categories
(Core :: Graphics, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: martin, Assigned: lsalzman)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
rhunt
:
review+
|
Details | Diff | Splinter Review |
Sparc64 used Skia for some time, but the 36 branch broke this and now Firefox does not build at all any more.
Reporter | ||
Comment 1•10 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!
Comment 2•10 years ago
|
||
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•10 years ago
|
||
I don't think 1005535 will work on sparc* w/o massive work (which I right now can not do).
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
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•10 years ago
|
||
(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•10 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.
Comment 10•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
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•10 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.
Comment 13•10 years ago
|
||
So you used --disable-skia-gpu i suppose ?
Reporter | ||
Comment 14•10 years ago
|
||
No, --disable-skia
We're about to make this worse. See bug 1323303.
Updated•8 years ago
|
Comment 16•7 years 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•7 years 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•7 years ago
|
Priority: -- → P3
Comment 18•7 years ago
|
||
(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 19•7 years ago
|
||
Attachment #8948259 -
Flags: review?(martin)
Reporter | ||
Comment 20•7 years 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+
Comment 21•7 years ago
|
||
(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•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
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•7 years 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•7 years 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+
Updated•7 years ago
|
Attachment #8960052 -
Flags: review?(milan) → review?(lsalzman)
Assignee | ||
Comment 26•7 years ago
|
||
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-
Comment 27•7 years ago
|
||
Attaching patch updated for the current development version.
Should probably try to get this upstreamed to skia.
Attachment #8960052 -
Attachment is obsolete: true
Comment 28•7 years ago
|
||
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•7 years ago
|
||
I can confirm that with your latest patch I can build Firefox on sparc.
Updated•6 years ago
|
Attachment #8969223 -
Flags: review?(lsalzman)
Assignee | ||
Comment 30•6 years ago
|
||
(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)
Comment 31•6 years ago
|
||
(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)
Assignee | ||
Comment 32•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8980162 -
Flags: review?(rhunt) → review+
Comment 33•6 years ago
|
||
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f26f8702696
fix big-endian Skia builds. r=rhunt
Comment 34•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee | ||
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•