1005535 - enable skia_gpu on big endian platforms

Status: RESOLVED INVALID

(Core :: Graphics, defect)

Description

[Steve Singer (:stevensn)]

We currently set MOZ_ENABLE_SKIA_GPU=0 on ppc,ppc64 and sparc even when building skia enabled.  We should allow for big endian platforms using skia to build the GPU code as well.

Comment 1

[Steve Singer (:stevensn)]

Attachment: skia_gpu_bendian.diff

This patch adds a configure time flag to indicate if the target platform is big endian or little endian.  This is then used in the moz.build files to set the proper defines for building skia gpu.

This patch is dependent on skia_gpu_bendian_upstream.diff

Comment 2

[Steve Singer (:stevensn)]

Attachment: skia_gpu_bendian_upstream.diff

These are the changes to the upstream provided files to remove the #error when building.

I have not yet patches

Comment 3

[Steve Singer (:stevensn)]

Attachment: skia_bigendian_alternate.diff

The previous patches actually rendered everything blue.
This patch builds with the skia GPU code and looks fine using skia a 64 bit big endian platform (ppc64).  I am not yet sure if the GPU code is actually tested or works though

Comment 4

[Steve Singer (:stevensn)]

  Commit https://hg.mozilla.org/mozilla-central/rev/3b5fb4abaa3f (part of Bug 1003707)
appears to have introduced an issue where parts of the background show up black or gray when running with gfx.content.azure.backends=skia (but not cario) with attachment 8420608 [details] [diff] [review] applied on my big endian machine.

Comment 5

[Steve Singer (:stevensn)]

Attachment: with_skia.png

Attaching a screenshot of how things look with this patch when using skia after commit https://hg.mozilla.org/mozilla-central/rev/3b5fb4abaa3f which introduced the black/gray.

Comment 6

[Cameron Kaiser [:spectre]]

I'd prefer this wasn't enabled for OS X/ppc -- Skia has issues on 10.4, and we still build with the 10.4 SDK. Eventually I'll get around to fixing them but for now we're fine with Cairo.

Comment 7

[Steve Singer (:stevensn)]

Cameron, 
attachment 8420608 [details] [diff] [review] should still allow you to build on OSX/PPC with --disable-skia (and that patch should not change the default for using/not using skia).  If USE_SKIA_GPU is set but USE_SKIA isn't set then I'd expect things to build fine without SKIA. Please correct me if I am mistaken.

Comment 8

[Steve Singer (:stevensn)]

Attachment: skia_bigendian_alternate.diff

Updated patch to fix bitrot

Comment 9

[Landry Breuil (:gaston)]

(In reply to Cameron Kaiser [:spectre] from comment #6)
> I'd prefer this wasn't enabled for OS X/ppc -- Skia has issues on 10.4, and
> we still build with the 10.4 SDK. Eventually I'll get around to fixing them
> but for now we're fine with Cairo.

Fwiw, i'm not sure this will still be possible soonish, given https://bugzilla.mozilla.org/show_bug.cgi?id=1105087#c2

Comment 10

[Landry Breuil (:gaston)]

Testing att 8528718 to see if it fixes my build-failures on m-c/m-a sparc64...

Comment 11

[Steve Singer (:stevensn)]

Attachment: skia_bigendian_alternate.diff

This version of the patch uses the same byte ordering on big endian as on little endian.  It seems to show things in the right color when using skia but I don't understand why (I would have expected to need different values for the SK_A32_SHIFT series of defines)

Comment 12

[Landry Breuil (:gaston)]

While testing, i realized that i had to add --enable-skia to get MOZ_ENABLE_SKIA set to 1 on sparc64, by default only tier1 archs have it enabled...

Comment 13

[Steve Singer (:stevensn)]

Attachment: skia_bigendian_alternate.diff

Attached is a cleaned up version of the patch that seems to work on ppc and ppc64.  I was a bit surprised that the same bit order works on big endian.
I also don't seem to need a ARGB32ToBGRA in the layer manager.


Jeff, does this approach make sense to you?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

I'm not sure this approach is workable in the long term. This would give us the burden of having to support a color format that's not supported in upstream Skia which I'm not thrilled with... It also means that big endian would have no hw accelerated skia support. Still, it might be the easiest thing to do in the short term.

Another idea I had would be to add support to cairo for the BGRA data format. It might be worth proposing this on the cairo list.

It's unfortunate that all of the options here are bad.

Comment 15

[Steve Singer (:stevensn)]

Jeff, as I understand the options we could

1) Go with this patch which is using SKIA in ARGB mode on big endian but using the BGRA enums (or am I naming this wrong)
2) Add support to cario for BGRA on big endian
3) Somehow do conversions between the format. I am not clear where we would do these conversions.  The layer manager functions you mentioned in bug 1105087# are not being called for normal SKIA draw operations(outside of stuff changed in bug  1097776)

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to Steve Singer (:stevensn) from comment #15)
> Jeff, as I understand the options we could
> 
> 1) Go with this patch which is using SKIA in ARGB mode on big endian but
> using the BGRA enums (or am I naming this wrong)

This approach probably won't work for GPU support. OpenGL doesn't have much support for the cairo big endian color ARGB format and my understanding is Skia assumes only BGRA or RGBA support.

> 2) Add support to cario for BGRA on big endian
> 3) Somehow do conversions between the format. I am not clear where we would
> do these conversions.  The layer manager functions you mentioned in bug
> 1105087# are not being called for normal SKIA draw operations(outside of
> stuff changed in bug  1097776)

That basically matches my understanding.

Comment 17

[Landry Breuil (:gaston)]

Is this stuck? since bug 1136958 landed some days ago the patch here isnt applying anymore on my sparc64 buildbot...

Comment 18

[Steve Singer (:stevensn)]

Yah this is stuck.

I can upload a refreshed patch of #8544332 but that approach doesn't seem acceptable 
I can upload a refreshed patch of  where so things build with skia but will crash on startup if skia is enabled (I don't see this getting us much)
I don't really have a good sense of how to find all the places that would need to do bit-swapping between cario and skia space if we wanted to go that route.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8544332 [details] [diff] [review]
skia_bigendian_alternate.diff

After a bit of discussion, gw280 and I decided that it doesn't make any sense to enable skia gpu on big endian without upstream support. If you're interested in pursuing this you should work on it in upstream skia.

Comment 20

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

To elaborate further, we don't even use Skia/GL on little endian platforms right now (with the exception of Android), and so I'm not sure I see where the desire comes from to enable it on BE platforms.

I'm talking with upstream at the moment to gauge their interest in porting to BE, but I suspect that the engineering effort will have to come from you guys as upstream don't have any BE testers or any internal desire to support BE. The discussion has been widened to their team at https://code.google.com/p/skia/issues/detail?id=3571

I'll close this issue as invalid as I don't think this is a Mozilla issue; if upstream supports BE then we will inherit the BE support, but we are not in a position to drive development of BE support in Skia, nor do we want to fork upstream.

Comment 21

[Cameron Kaiser [:spectre]]

Upstream today said they weren't interested, in any case, so that's that.