1105087 - build failure with --disable-skia

Status: RESOLVED DUPLICATE of bug 1136958

(Core :: Graphics, defect)

Description

[Steve Singer (:stevensn)]

Some non-tier 1 platforms (sparc and ppc) are building by default with SKIA disabled. These builds are now failing with

gfx/layers/basic/BasicLayerManager.cpp:49:56: fatal error: skia/SkCanvas.h: No such file or directory
 2:45.28  #include "skia/SkCanvas.h"              // for SkCanvas
 2:45.28                                                         ^
 2:45.28 compilation terminated.

This appears to have been introduced with bug 1097776

Comment 1

[Steve Singer (:stevensn)]

Markus,

Was it your intention with bug 1097776 to make SKIA required  or was that inadvertent.
I had heard talk that skia might be required on all platforms but I am not sure where that stands.

Comment 2

[Markus Stange [:mstange]]

I was assuming Skia was already required everywhere, I didn't know we had a --disable-skia flag.
Jeff, should we remove that flag?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Yeah, I think it makes sense to remove that flag. However, depending on the burden maybe it would be easier to just have cairo and skia paths here. Is the reason sparc and ppc build with Skia disabled big endian? What work is necessary to enable skia on sparc and ppc?

Comment 4

[Steve Singer (:stevensn)]

The patch on bug 1005535 will get skia compiling on ppc but it is not actually working.
For example the right hand pane on the site mentioned in bug 1097776 is just blank until I mouse over it.
It is probably an endian issue but I'm not sure where it lies.

If I turn skia on in about:config  gfx.content.azure.backends
I get crash on startup
 gfxPlatform::Init ()

if (!gPlatform->mScreenReferenceDrawTarget) {
506           NS_RUNTIMEABORT("Could not initialize mScreenReferenceDrawTarget");
507         }

I think because
SkBitmapDevice::Create is failing but I am not yet sure why

Comment 5

[Steve Singer (:stevensn)]

If I force  kN32_SkColorType = kBGRA_8888_SkColorType then right hand pane at http://christianheilmann.com/2014/11/05/taking-a-break/ works.   If I switch gfx.content.azure.backends=skia everything shows up in blue but otherwise works if I turn off e10, but I just get a blank screen with e10 on (e10 works if I am not using skia)

If kN32_SkColorType = kRGBA_8888_SkColorType it fails in SkBitmapDevice.cpp - valid_for_bitmap_device

I am not sure if kN32_SkColorType needs to be kRGBA_8888_SkColorType on big endian , and I need to fix things elsewhere so it doesn't pass in kBGRA_8888_SkColorType or if we should be using the  kBGRA_8888_SkColorType space.

Jeff do you have any advice?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

So the problem is that on big endian cairo and skia don't share a compatible byte order.

Cairo supports BGRA on little endian and ARGB on big endian
Skia supports BGRA on big and little endian and RGBA on big and little endian

This basically means we can't support Cairo/Skia interop on big endian without doing a byte swap operation.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

The best way forward might be to add functions ARGB32ToBGRA and BGRAToARGB32 that do nothing
on little endian and do a byteswap on big endian.

And then add something like this:

diff --git a/gfx/layers/basic/BasicCompositor.cpp b/gfx/layers/basic/BasicCompositor.cpp
index 0bef076..bfbfe3f 100644
--- a/gfx/layers/basic/BasicCompositor.cpp
+++ b/gfx/layers/basic/BasicCompositor.cpp
@@ -194,6 +194,8 @@ SkiaTransform(DataSourceSurface* aDest,
   if (aTransform.IsSingular()) {
     return;
   }
+  ARGB32ToBGRA(aDest->Data());
+  ARGB32ToBGRA(aSrc->GetData());
 
   IntSize destSize = aDest->GetSize();
   SkImageInfo destInfo = SkImageInfo::Make(destSize.width,
@@ -224,6 +226,9 @@ SkiaTransform(DataSourceSurface* aDest,
   paint.setFilterLevel(SkPaint::kLow_FilterLevel);
   SkRect destRect = SkRect::MakeXYWH(0, 0, srcSize.width, srcSize.height);
   destCanvas.drawBitmapRectToRect(src, nullptr, destRect, &paint);
+
+  BGRAToARGB32(aSrc->GetData());
+  BGRAToARGB32(aDest->Data());
 }
 
 static inline IntRect
diff --git a/gfx/layers/basic/BasicLayerManager.cpp b/gfx/layers/basic/BasicLayerManager.cpp
index 5162130..a08b268 100644
--- a/gfx/layers/basic/BasicLayerManager.cpp
+++ b/gfx/layers/basic/BasicLayerManager.cpp
@@ -628,6 +628,8 @@ SkiaTransform(const gfxImageSurface* aDest,
     return;
   }
 
+  ARGB32ToBGRA(aDest->Data());
+  ARGB32ToBGRA(aSrc->GetData());
   IntSize destSize = ToIntSize(aDest->GetSize());
   SkImageInfo destInfo = SkImageInfo::Make(destSize.width,
                                            destSize.height,
@@ -657,6 +659,8 @@ SkiaTransform(const gfxImageSurface* aDest,
   paint.setFilterLevel(SkPaint::kLow_FilterLevel);
   SkRect destRect = SkRect::MakeXYWH(0, 0, srcSize.width, srcSize.height);
   destCanvas.drawBitmapRectToRect(src, nullptr, destRect, &paint);
+  BGRAToARGB32(aSrc->GetData());
+  BGRAToARGB32(aDest->Data());
 }
 
 /**

Comment 8

[Landry Breuil (:gaston)]

Fwiw, aurora and central dont build on sparc64 because of this - checking beta now but given that 1097776 landed in 36, beta should be fine. Oh, and the day skia becomes mandatory (and is thus "supported"/"working" on BE archs) i think that'll allow us to remove quite some #if around...

Comment 9

[Cameron Kaiser [:spectre]]

I'm thinking for TenFourFox, since Skia is still a bag of hurt on 10.4, I'm just going to back 1097776 out.

Comment 10

[Cameron Kaiser [:spectre]]

Jeff, I've done some toying with the idea above and having to do four byte swaps per operation really chugs. Do you or Markus mind terribly having a Cairo path?

If you're ok with a Cairo path, I'll write up a patch for that (essentially has the old code, only if --disable-skia).
If you'd prefer not, I'll just keep bug 1097776 backed out of TenFourFox until I can't avoid it, since the Cairo code is faster (uglier, but quick is better on these low spec Macs).

Comment 11

[Cameron Kaiser [:spectre]]

Attachment: Patch proposal (against aurora, for feedback only)

Like this.

Comment 12

[pbrobinson]

(In reply to Steve Singer (:stevensn) from comment #0)
> Some non-tier 1 platforms (sparc and ppc) are building by default with SKIA
> disabled. These builds are now failing with

Failing also on aarch64 on Linux (Fedora at least)

Comment 13

[Martin Husemann]

(In reply to Steve Singer (:stevensn) from comment #0)
> Some non-tier 1 platforms (sparc and ppc) are building by default with SKIA
> disabled. These builds are now failing with

Just for the record, NetBSD/sparc64 has build with skia enabled since ages, and skia works just fine there (after #884376 got patched).

But now the configure.in change skips it there, which makes the build fail.

Comment 14

[Martin Husemann]

Comment on attachment 8543177 [details] [diff] [review]
Patch proposal (against aurora, for feedback only)

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

Minor nit: https://bugzilla.mozilla.org/attachment.cgi?id=8543177&action=diff#a/gfx/layers/basic/BasicLayerManager.cpp_sec5
needs a s/PixmanTransform/DrawTransform/

Comment 15

[Cameron Kaiser [:spectre]]

Actually, Mike's patch is almost exactly the same in bug 1136958, and this is really the same bug anyway, so duping.