Closed Bug 1105087 Opened 5 years ago Closed 5 years ago

build failure with --disable-skia

Categories

(Core :: Graphics, defect)

PowerPC
Linux
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1136958
Tracking Status
firefox35 --- ?
firefox36 --- affected
firefox37 --- affected

People

(Reporter: stevensn, Unassigned)

References

Details

Attachments

(1 file)

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.
Flags: needinfo?(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?
Flags: needinfo?(mstange) → needinfo?(jmuizelaar)
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?
Flags: needinfo?(jmuizelaar) → needinfo?(steve)
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
Flags: needinfo?(steve)
Blocks: 1097776
No longer blocks: 109776
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?
Flags: needinfo?(jmuizelaar)
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.
Flags: needinfo?(jmuizelaar)
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).
Flags: needinfo?(jmuizelaar)
Like this.
Flags: needinfo?(jmuizelaar)
Attachment #8543177 - Flags: feedback?(jmuizelaar)
(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.
See Also: → 1144632
Depends on: 1144632
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1136958
Attachment #8543177 - Flags: feedback?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.