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

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.

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

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?

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

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?

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.

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()); } /**

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...

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

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).

Like this.

(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)

(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 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/

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