884376 - Skia assumes sparc is a little endia cpu

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Martin Husemann]

Attachment: add sparc to the list of big endian cpus

.. but certainly sparc is big endian

Comment 1

[Martin Husemann]

Furthermore SKIA_GPU has to be disabled for sparc as well, something like this to configure.in should do it:

@@ -8352,7 +8369,7 @@
     AC_DEFINE(GR_DLL)
   fi
 
-  if test "${CPU_ARCH}" != "ppc" -a "${CPU_ARCH}" != "ppc64"; then
+  if test "${CPU_ARCH}" != "ppc" -a "${CPU_ARCH}" != "ppc64" -a "${CPU_ARCH}" != "sparc"; then
     MOZ_ENABLE_SKIA_GPU=1
     AC_DEFINE(USE_SKIA_GPU)
     AC_SUBST(MOZ_ENABLE_SKIA_GPU)

Comment 2

[Landry Breuil (:gaston)]

I'd rather have it as a whitelist of archs supporting skia/gpu (ie tested on i386/amd64/arm ?) or based on endianness or... rather than having to blacklist every BE arch.

See also bug 849253. As for the patch against skia itself, it should either complete gfx/skia/patches/0018-Bug-817356-PPC-defines.patch or be made on top of it, so that we keep track of changes against upstream..

Comment 3

[Martin Husemann]

Also reported upstream: http://code.google.com/p/skia/issues/detail?id=1355

Comment 5

[Steve Singer (:stevensn)]

Attachment: add more platforms to SKIA Big Endian list

This patch updates the patch in gfx/skia/patches
It also adds other platforms to the list in SkPreConfig.h based on what we do in mfbt/Endian.h

I haven't update configure.in to use a whitelist for SKIA_GPU I can do that if we want

Comment 6

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

Comment on attachment 8351431 [details] [diff] [review]
add more platforms to SKIA Big Endian list

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

I'm happy for the change to configure.in to land immediately, but please upstream the remaining hunks first as we now have an upstream-first policy for patches.

Comment 7

[Steve Singer (:stevensn)]

Patch submitted upstream https://codereview.appspot.com/6900063/

Comment 8

[Steve Singer (:stevensn)]

Wrong upstream URL.

https://codereview.chromium.org/174603002

Comment 9

[Steve Singer (:stevensn)]

Attachment: configure.in changes for sparc

Split out the configure.in portion of the patch

Comment 10

[Steve Singer (:stevensn)]

Attachment: 884376_skia.upstream.diff

the patches to the upstream components

Comment 11

[Steve Singer (:stevensn)]

checkin requested for Attachment #8387343 [details] [diff]

Comment 12

[Landry Breuil (:gaston)]

(In reply to Steve Singer (:stevensn) from comment #11)
> checkin requested for Attachment #8387343 [details] [diff] [diff]

a bit late to the party, but could we add sparc64 in there too ?

Comment 13

[Martin Husemann]

I'm pretty sure in this context ${CPU_ARCH} is "sparc" for sparc64, at least on NetBSD.

Comment 14

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

If the patch has been upstreamed there's no need to keep it around in gfx/skia/patches. I will be pruning that directory soon anyway.

Comment 15

[Landry Breuil (:gaston)]

(In reply to Martin Husemann from comment #13)
> I'm pretty sure in this context ${CPU_ARCH} is "sparc" for sparc64, at least
> on NetBSD.

you're right, it's set a little bit earlier in configure by

1171 sun4u | sparc*)
1172     CPU_ARCH=sparc
1173     ;;

Comment 16

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81c5d568d949

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81c5d568d949

Comment 18

[Steve Singer (:stevensn)]

(In reply to George Wright (:gw280) from comment #14)
> If the patch has been upstreamed there's no need to keep it around in
> gfx/skia/patches. I will be pruning that directory soon anyway.

I've submitted the patch upstream but it hasn't been applied yet.

Any hints on getting the patch moved through the SKIA reveiew/commit process?

Comment 19

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

(In reply to Steve Singer (:stevensn) from comment #18)
> Any hints on getting the patch moved through the SKIA reveiew/commit process?

Flag [e-mail] as the reviewer and ensure the "send mail to reviewers" option is checked.