849253 - skia doesnt build on big endian arch

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Landry Breuil (:gaston)]

eg++ -o Factory.o -c  -fvisibility=hidden -DMOZ_GFX -DUSE_CAIRO -DGFX2D_INTERNAL -DMOZ_ENABLE_FREETYPE -DSK_A32_SHIFT=24 -DSK_R32_SHIFT=16 -DSK_G32_SHIFT=8 -DSK_B32_SHIFT
=0 -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEB
ES -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DEXCLUDE_SKIA_DEPENDENCIES -DOS_POSIX=1 -DOS_OPENBSD=1 -DOS_BSD=1 -I/home/landry/src/mozilla-central/ipc/chromium/src 
-I/home/landry/src/mozilla-central/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/home/landry/src/mozilla-central/gfx/2d -I. -I../../dist/include  -I/usr/obj/m-c/dist/include
/nspr -I/usr/obj/m-c/dist/include/nss      -fPIC -I/usr/X11R6/include -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-inval
id-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMME
D -g -fno-omit-frame-pointer  -I/usr/obj/m-c/dist/include/cairo   -I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/Factory.o.pp  /home/
landry/src/mozilla-central/gfx/2d/Factory.cpp
In file included from ../../dist/include/skia/GrSurface.h:12:0,
                 from ../../dist/include/skia/GrTexture.h:12,
                 from ../../dist/include/skia/GrPaint.h:13,
                 from ../../dist/include/skia/GrContext.h:14,
                 from /home/landry/src/mozilla-central/gfx/2d/DrawTargetSkia.h:8,
                 from /home/landry/src/mozilla-central/gfx/2d/Factory.cpp:14:
../../dist/include/skia/GrTypes.h:303:6: error: #error "Skia gpu currently assumes little endian"


Since a recent commit to skia it doesnt build anymore on powerpc. I've done a success build of m-c around the 24 of february.

Comment 1

[Landry Breuil (:gaston)]

Looking at what got recently commited, looks like a fallout of 751418. Should USE_SKIA be false on BE archs ?

Comment 2

[Landry Breuil (:gaston)]

Now i'm pretty sure it's https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6914fccc4f that is causing this - previously the gpu support in skia wasnt enabled so code under src/gpu wasnt reached, and now it is.

Now, what would be the best way to fix it ?

Comment 3

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

Don't build $(GPU_CPPSRCS) and $(GPU_GL_CPPSRCS) on big endian, and only define SK_SUPPORT_GPU on little endian platforms, I suspect.

Comment 4

[Landry Breuil (:gaston)]

(In reply to George Wright (:gw280) from comment #3)
> Don't build $(GPU_CPPSRCS) and $(GPU_GL_CPPSRCS) on big endian, and only
> define SK_SUPPORT_GPU on little endian platforms, I suspect.

Tried only #define SK_SUPPORT_GPU 0 first & then commenting out GPU_CPPSRCS/GPU_GL_CPPSRCS (with the #define still off), both failed at the same spot.  Changing that doesnt modify the way 2d/Factory.cpp include GrContext.h via DrawTargetSkia.h.....

Comment 5

[Landry Breuil (:gaston)]

Looking at http://code.google.com/p/skia/issues/detail?id=524 it seems upstream skia doesnt really care at all about big endian. So a proper way to disable skia/gpu on BE would be the way to go.

Comment 6

[Landry Breuil (:gaston)]

Tried disabling skia/gpu in the build itself wont fly, it seems like way too much #ifdef machinery. I see two options :
- Find the correct kSkia8888_GrPixelConfig value for BE archs and remove the #error ?
- remove the #error and return false in gfx/thebes/gfxPlatform.cpp:UseAcceleratedSkiaCanvas() ?

Comment 7

[Steve Singer (:stevensn)]

I removed the #ifndef  SK_CPU_LENDIAN check and got a firefox build.
Nothing obvious is broken after visiting a few sites.  Is there anything special (paramaters, or web sites that do fancy canvas stuff) I need to do to get skia to use the GPU?

Some of the SKIA code is BE aware, ie SkConfig8888.cpp has support for BE packings.

Comment 8

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

gfx.canvas.azure.backends = skia
gfx.canvas.azure.accelerated = true

That will enable the OpenGL backend for Skia. I suspect it won't work (or at least, will have R/B swapped).

Comment 9

[Landry Breuil (:gaston)]

(In reply to steve from comment #7)
> I removed the #ifndef  SK_CPU_LENDIAN check and got a firefox build.
> Nothing obvious is broken after visiting a few sites.  Is there anything
> special (paramaters, or web sites that do fancy canvas stuff) I need to do
> to get skia to use the GPU?

Care to share a diff ? I tried this, and it failed to build - but that might have been more unrelated fiddling i did locally.

(In reply to George Wright (:gw280) from comment #8)
> gfx.canvas.azure.backends = skia
> gfx.canvas.azure.accelerated = true
> 
> That will enable the OpenGL backend for Skia. I suspect it won't work (or at
> least, will have R/B swapped).

If it doesnt work, maybe it should return false in gfx/thebes/gfxPlatform.cpp:UseAcceleratedSkiaCanvas() on BE archs to be on the safe side.

Comment 10

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

Attachment: Build SkiaGL independently

Here's an untested patch that makes SkiaGL independently buildable from Skia itself, and disabled by default on PPC. Please test as I don't have a PPC machine handy.

Comment 11

[Steve Singer (:stevensn)]

Attachment: get SKIA to compile on BE.

Attached is a patch that gets firefox compiling on ppc with SKIA.  Firefox works fine when SKIA is disabled but forcing azure to SKIA gives me segfaults in  mozilla::dom::CanvasRenderingContext2D::EnsureTarget  because GLContextProviderGLX::CreateOffscreen is returning nullptr.  I do not know if that is because of BE or unrelated issues with my graphics driver

Comment 12

[Steve Singer (:stevensn)]

I should also add that I had my patch from 846986 applied so I could link. 

I will test patch 724672 but the build will take a while

Comment 13

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

My patch is currently a little broken. It doesn't define MOZ_ENABLE_SKIA_GPU properly. I'll try and fix that later today.

I don't think your patch is going to work, unfortunately.

Comment 14

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

Attachment: Updated patch

OK, this one should work...

Comment 15

[Steve Singer (:stevensn)]

The updated patch doesn't build.

ake[6]: Entering directory `/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/gfx/2d'
c++ -o Factory.o -c  -fvisibility=hidden -DMOZ_GFX -DUSE_CAIRO -DGFX2D_INTERNAL -DMOZ_ENABLE_FREETYPE -DSK_A32_SHIFT=24 -DSK_R32_SHIFT=16 -DSK_G32_SHIFT=8 -DSK_B32_SHIFT=0 -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DNO_NSPR_10_SUPPORT -DEXCLUDE_SKIA_DEPENDENCIES -DOS_POSIX=1 -DOS_LINUX=1 -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/ipc/chromium/src -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/gfx/2d -I. -I../../dist/include  -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/dist/include/nspr -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/dist/include/nss      -fPIC  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks  -fomit-frame-pointer -I/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/dist/include/cairo    -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/Factory.o.pp  /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/gfx/2d/Factory.cpp
In file included from ../../dist/include/skia/GrSurface.h:12,
                 from ../../dist/include/skia/GrTexture.h:12,
                 from ../../dist/include/skia/GrPaint.h:13,
                 from ../../dist/include/skia/GrContext.h:14,
                 from /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/gfx/2d/DrawTargetSkia.h:8,
                 from /media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/gfx/2d/Factory.cpp:14:
../../dist/include/skia/GrTypes.h:303:6: error: #error "Skia gpu currently assumes little endian"


grep SKIA config.status
    (''' MOZ_ENABLE_SKIA ''', ' 1 '),
    (''' USE_SKIA ''', ' 1 '),
    (''' MOZ_ENABLE_SKIA_GPU ''', r'''  '''),
    (''' MOZ_ENABLE_SKIA ''', r''' 1 '''),
    (''' MOZ_SKIA_LIBS ''', r''' $(DEPTH)/gfx/skia/$(LIB_PREFIX)skia.$(LIB_SUFFIX) '''),

Comment 16

[Steve Singer (:stevensn)]

Was your patch intended to be applied on-top of the one I posted?

Comment 17

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

(In reply to steve from comment #16)
> Was your patch intended to be applied on-top of the one I posted?

Nope. What architecture are you on?

Comment 18

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

Oh, I see what's wrong. Can you change:

#include "skia/GrContext.h"
#include "skia/SkCanvas.h"
#include "skia/GrContext.h"
#include "skia/GrGLInterface.h"

in DrawTargetSkia.h to:

#ifdef USE_SKIA_GPU
#include "skia/GrContext.h"
#include "skia/GrContext.h"
#include "skia/GrGLInterface.h"
#endif
#include "skia/SkCanvas.h"

Comment 19

[Steve Singer (:stevensn)]

Attachment: Updated patch v3

In addition to DrawTargetSkia.h I had to do something similar in HelpersSkia.h

I also had to make sure gets defined SK_SUPPORT_GPU 0 otherwise it was pulling defaulting to 1 in SkPostConfig.h and was pulling in GrTypes.h

Attached is an updated version of your patch with this changes.  It seems to compile but I haven't really tested it yet

Comment 20

[Landry Breuil (:gaston)]

I tried testing it but bug #840282 broke powerpc. (again.sigh.)

Comment 21

[Steve Singer (:stevensn)]

Firefox seems to be working fine with the patch. I haven't updated my repository since startinng on this so I am not yet hit by 840282

Comment 22

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

https://tbpl.mozilla.org/?tree=Try&rev=e88442228841

Comment 23

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

Attachment: patch v4

Final patch (hopefully) that's currently running through try.

Comment 24

[Landry Breuil (:gaston)]

Firefox builds fine but crashes at startup for me with att 725431, but that might be related to #840282 - if it works for steve, that should be good for me too.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 726436 [details] [diff] [review]
patch v4

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

::: configure.in
@@ +8286,5 @@
>    fi
> +
> +  if test "${CPU_ARCH}" != "ppc" -a "${CPU_ARCH}" != "ppc64"; then
> +    MOZ_ENABLE_SKIA_GPU=1
> +    AC_DEFINE(USE_SKIA_GPU)

Why MOZ_ for the makefile define and not for the c++ define?

Comment 26

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> Comment on attachment 726436 [details] [diff] [review]
> patch v4
> 
> Review of attachment 726436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +8286,5 @@
> >    fi
> > +
> > +  if test "${CPU_ARCH}" != "ppc" -a "${CPU_ARCH}" != "ppc64"; then
> > +    MOZ_ENABLE_SKIA_GPU=1
> > +    AC_DEFINE(USE_SKIA_GPU)
> 
> Why MOZ_ for the makefile define and not for the c++ define?

Mainly because that's what we currently do for the generic Skia defines. I filed bug 851212 related to this.

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2bfeaaecaa

Comment 28

[Steve Singer (:stevensn)]

I am still getting build failures with this.

central/gfx/2d/Scale.cpp
In file included from /media/nfs/usb_drive_src/firefox/mozilla-central/gfx/2d/HelpersSkia.h:13:0,
                 from /media/nfs/usb_drive_src/firefox/mozilla-central/gfx/2d/Scale.cpp:8:
../../dist/include/skia/GrTypes.h:303:6: error: #error "Skia gpu currently assumes little endian"

My attachement 725431 had changes to HelpersSkia.h that aren't in the patch that got committed

Comment 29

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/ee2bfeaaecaa

Comment 30

[Landry Breuil (:gaston)]

Attachment: Missing chunk from patch v4 on HelpersSkia.h

I concur - you need the following missing chunk to build on BE.

Comment 31

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2fb60b72822

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2fb60b72822