Get Azure/Skia content rendering working on Linux

RESOLVED FIXED in mozilla27

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: gw280, Assigned: kentuckyfriedtakahe)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla27
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 13 obsolete attachments)

4.67 KB, patch
Details | Diff | Splinter Review
4.59 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
6.96 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
11.48 KB, patch
roc
: review+
Details | Diff | Splinter Review
1010 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Tracking bug for Skia/Linux progress (content rendering)
(Reporter)

Updated

6 years ago
Depends on: 740202
(Reporter)

Updated

6 years ago
Depends on: 740580
(Reporter)

Updated

6 years ago
Depends on: 740560
Created attachment 610688 [details] [diff] [review]
Bug 740200 - Initial work for Skia content rendering on Linux
Attachment #610688 - Flags: review?(matt.woodrow)
Attachment #610688 - Flags: review?(jmuizelaar)
Comment on attachment 610688 [details] [diff] [review]
Bug 740200 - Initial work for Skia content rendering on Linux

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

- because the some of the ownership nsWindow is not clear.

::: gfx/thebes/gfxXlibNativeRenderer.cpp
@@ +590,5 @@
> +        gfxMatrix matrix = ctx->CurrentMatrix();
> +        ctx = new gfxContext(target);
> +        ctx->SetMatrix(matrix);
> +    } else {
> +        return;

I'd rather us crash here than blindly return.

@@ +595,5 @@
> +    }
> +
> +    // This handles locking/unlocking the bitmap pixel buffer for us
> +    BitmapPixelManager pixelManager(bitmap);
> +

It looks like this requires us build with Skia. Do we want this?

::: widget/gtk2/nsWindow.cpp
@@ +2187,5 @@
> +    if (gfxPlatform::UseAzureContentDrawing()) {
> +        gfxImageSurface* imgSurf = static_cast<gfxImageSurface*>(surf);
> +
> +        // There's no helper function to convert from gfxIntSize to mozilla::gfx::IntSize :(
> +        mozilla::gfx::IntSize size(imgSurf->GetSize().width, imgSurf->GetSize().height);

Please add the helper.

@@ +2198,5 @@
> +                                                                imgSurf->Stride(),
> +                                                                format);
> +
> +        targetSurface->AddUserData(&kThebesSurfaceKey, imgSurf, DestroyThebesSurface);
> +

Does this create a circular reference?

@@ +2199,5 @@
> +                                                                format);
> +
> +        targetSurface->AddUserData(&kThebesSurfaceKey, imgSurf, DestroyThebesSurface);
> +
> +        imgSurf->AddRef();

Why is this here? What ownership does it represent?
Attachment #610688 - Flags: review?(jmuizelaar) → review-
Status: NEW → ASSIGNED
QA Contact: ajones
Attachment #610688 - Flags: review?(matt.woodrow)

Comment 3

5 years ago
Try run for acbb7ed39636 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=acbb7ed39636
Results (out of 1 total builds):
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-acbb7ed39636

Comment 4

5 years ago
Try run for acbb7ed39636 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=acbb7ed39636
Results (out of 1 total builds):
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-acbb7ed39636

Comment 5

5 years ago
Try run for 6c800779d950 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6c800779d950
Results (out of 12 total builds):
    success: 9
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-6c800779d950
Created attachment 661104 [details] [diff] [review]
Part 1: Whitespace changes
Created attachment 661105 [details] [diff] [review]
Part 2: DrawTargetCairo support
Created attachment 661106 [details] [diff] [review]
Part 3: DrawTargetSkia support
Created attachment 661113 [details] [diff] [review]
Part 4: Cairo and Skia in nsWindow and gfxXlibNativeRenderer
Attachment #610688 - Attachment is obsolete: true

Comment 10

5 years ago
Try run for f6de0ea5a3fa is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f6de0ea5a3fa
Results (out of 98 total builds):
    exception: 4
    success: 74
    warnings: 20
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-f6de0ea5a3fa
Created attachment 665778 [details] [diff] [review]
Part 2: DrawTargetCairo support v2
Attachment #661105 - Attachment is obsolete: true
Attachment #665778 - Flags: review?(jmuizelaar)
Created attachment 665779 [details] [diff] [review]
Part 3: DrawTargetSkia support v2
Attachment #661106 - Attachment is obsolete: true
Attachment #665779 - Flags: review?(jmuizelaar)
Created attachment 665781 [details] [diff] [review]
Part 4: Cairo and Skia in nsWindow and gfxXlibNativeRenderer v2
Attachment #665781 - Flags: review?(jmuizelaar)

Comment 14

5 years ago
Try run for a3b4b495b3e6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a3b4b495b3e6
Results (out of 90 total builds):
    success: 80
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-a3b4b495b3e6

Comment 15

5 years ago
Try run for a3b4b495b3e6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a3b4b495b3e6
Results (out of 97 total builds):
    success: 83
    warnings: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-a3b4b495b3e6
Depends on: 795763, 753237, 750434
Attachment #661113 - Attachment is obsolete: true
Comment on attachment 665778 [details] [diff] [review]
Part 2: DrawTargetCairo support v2

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +886,5 @@
>      return cairo_get_target(mContext);
>    }
> +  if (aType == NATIVE_SURFACE_CAIRO_CONTEXT) {
> +    return mContext;
> +  }

I think this would be better done with bug 790673

::: gfx/thebes/gfxPlatform.cpp
@@ +482,5 @@
>  RefPtr<DrawTarget>
>  gfxPlatform::CreateDrawTargetForSurface(gfxASurface *aSurface, const IntSize& aSize)
>  {
> +  if (!aSurface->CairoSurface()) {
> +    return nullptr;

When does this occur and do we really want to just send nullptr on?
Attachment #665778 - Flags: review?(jmuizelaar) → review-
Comment on attachment 665779 [details] [diff] [review]
Part 3: DrawTargetSkia support v2

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

Ok

::: gfx/thebes/gfxXlibNativeRenderer.h
@@ +94,1 @@
>  

Unintentional change?
Attachment #665779 - Flags: review?(jmuizelaar) → review+
Depends on: 790673
Comment on attachment 665781 [details] [diff] [review]
Part 4: Cairo and Skia in nsWindow and gfxXlibNativeRenderer v2

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

::: gfx/thebes/gfxXlibNativeRenderer.cpp
@@ +3,3 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

There seem to be a bunch of cosmetic changes that are distracting.

@@ +528,5 @@
> +    if (ctx->IsCairo()) {
> +        nsRefPtr<gfxASurface> surface(ctx->CurrentSurface());
> +        DrawFallback(ctx, surface, size, drawingRect, canDrawOverBackground,
> +                     flags, screen, visual, result);
> +    } else {

Seems like the else branch should be in a separate function.

::: gfx/thebes/gfxXlibNativeRenderer.h
@@ +13,5 @@
>  
>  class gfxASurface;
>  class gfxXlibSurface;
>  class gfxContext;
> +typedef struct _cairo cairo_t;

What is this needed for?

::: widget/gtk2/nsWindow.cpp
@@ +2146,5 @@
> +        mozilla::gfx::IntSize intSize(size.width, size.height);
> +        mozilla::RefPtr<mozilla::gfx::DrawTarget> drawTarget =
> +            gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surf, intSize);
> +
> +        if (!drawTarget) {

When does this fail?

@@ +2268,1 @@
>                  }

This code should not duplicate all of the painting. It seems like you just need a function that returns a different gfxContext. It might also be good to move this alpha extraction code into a separate function.
Attachment #665781 - Flags: review?(jmuizelaar) → review-
Depends on: 840874

Comment 19

4 years ago
FWIW.
This year's js1k winner, is another example of a demo that is unusably slow in Firefox, fast in Chrome.
http://js1k.com/2013-spring/demo/1555

Until I set gfx.canvas.azure.backends;skia  ofc.

Also, I don't know if this is relevent, but I routinely get:
SkFontHost::GetFileName unimplemented
SkFontHost::GetFileName unimplemented
SkFontHost::GetFileName unimplemented
--- no context for glyph 15e1

on stderr.

Also, occasional lockup, although might be a lockup related to webgl demos (goes totally unresponsive, have to kill it).  Haven't tried profiling that yet.  Probably related to this popping up on stderr.
firefox: tpp.c:63: __pthread_tpp_change_priority: Assertion `new_prio == -1 || (new_prio >= __sched_fifo_min_prio && new_prio <= __sched_fifo_max_prio)' failed.
(In reply to nemo from comment #19)
> Also, I don't know if this is relevent, but I routinely get:
> SkFontHost::GetFileName unimplemented
> SkFontHost::GetFileName unimplemented
> SkFontHost::GetFileName unimplemented
> --- no context for glyph 15e1

The glyph issues should be fixed once bug 736276 lands
Blocks: 561361
Blocks: 716121
Blocks: 722186
Attachment #665778 - Attachment is obsolete: true
Attachment #665779 - Attachment is obsolete: true
Attachment #665781 - Attachment is obsolete: true
Created attachment 800435 [details] [diff] [review]
BorrowedContext support for cairo;
Attachment #800435 - Flags: review?(jmuizelaar)
Assignee: gwright → ajones
Created attachment 800438 [details] [diff] [review]
Azure content rendering on Linux;
Attachment #800438 - Flags: review?(jmuizelaar)
Created attachment 800440 [details] [diff] [review]
Azure support in nsWindow;
Attachment #800440 - Flags: review?(jmuizelaar)
Comment on attachment 800435 [details] [diff] [review]
BorrowedContext support for cairo;

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +1082,5 @@
> +    return nullptr;
> +  }
> +
> +  return static_cast<cairo_t*>(
> +    aDT->GetNativeSurface(NATIVE_SURFACE_CAIRO_CONTEXT));

This should probably null out the cairo context for aDT and WillChange() the DrawTarget like CoreGraphics one does.

@@ +1088,5 @@
> +
> +void
> +BorrowedCairoContext::ReturnCairoContextToDrawTarget(DrawTarget* aDT)
> +{
> +  aDT->SetTransform(aDT->GetTransform());

This could use a comment explaining why this does anything at all.
Attachment #800435 - Flags: review?(jmuizelaar) → review-
Created attachment 806411 [details] [diff] [review]
BorrowedContext support for cairo

I probably should have looked closer at the CG implementation. Now the Cairo version is very similar. I would still probably perfer to use the loan pattern with lambda syntax.
Attachment #800435 - Attachment is obsolete: true
Attachment #806411 - Flags: review?(jmuizelaar)
https://tbpl.mozilla.org/?tree=Try&rev=299d46f5fa4a
Comment on attachment 806411 [details] [diff] [review]
BorrowedContext support for cairo

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

::: gfx/2d/2D.h
@@ +1025,5 @@
> + *
> + * Callers should check the cr member after constructing the object
> + * to see if it succeeded. The DrawTarget should not be used while
> + * the context is borrowed. */
> +class BorrowedCairoContext

I wonder if we should split these helpers classes into their own header at some point, 2D.h is massive.

@@ +1066,5 @@
> +  ~BorrowedCairoContext() {
> +    MOZ_ASSERT(!cr);
> +  }
> +
> +  cairo_t *cr;

mCairo? Or something that gives cr and mDT consistent naming style.

::: gfx/2d/DrawTargetCairo.cpp
@@ +1101,5 @@
> +{
> +  if (aDT->GetType() != BACKEND_CAIRO) {
> +    return nullptr;
> +  }
> +  DrawTargetCairo* cairoDT = static_cast<DrawTargetCairo*>(aDT);

We still have the problem that DrawTargetRecording and DrawTargetDual can return BACKEND_CAIRO for GetType(), but this will be an invalid cast.

We have IsDualDrawTarget() on DrawTarget, I think we should add IsRecordingDrawTarget too. And then guard to make sure we don't do dumb things here.

@@ +1108,5 @@
> +
> +  // save the state to make it easier for callers to avoid mucking with things
> +  cairo_save(cairoDT->mContext);
> +
> +  // Newter the DrawTarget while the context is being borrowed

neuter?
Comment on attachment 800440 [details] [diff] [review]
Azure support in nsWindow;

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

::: widget/gtk2/nsWindow.cpp
@@ +2109,4 @@
>  #endif
>  
> +    nsRefPtr<gfxContext> ctx;
> +    if (gfxPlatform::GetPlatform()->SupportsAzureContent()) {

SupportsAzureContentForType(BACKEND_CAIRO)

@@ +2234,5 @@
> +{
> +  if (gfxPlatform::GetPlatform()->SupportsAzureContent()) {
> +      // We need to create our own buffer to force the stride to match the
> +      // expected stride.
> +      int32_t stride = GetBitmapStride(aBoundsRect.width) * 8;

I think i'd prefer to use gfx::GetAlignedStride<4>(gfx::BytesPerPixel(FORMAT_A8) * aBoundsRect.width);, since we only need to align to 4.
Attachment #800440 - Flags: review?(jmuizelaar) → review+
Created attachment 806945 [details] [diff] [review]
BorrowedContext support for cairo;
Attachment #806945 - Flags: review?(matt.woodrow)
Attachment #806411 - Attachment is obsolete: true
Attachment #806411 - Flags: review?(jmuizelaar)
Blocks: 918138
Created attachment 807019 [details] [diff] [review]
Removed some white space
Attachment #661104 - Attachment is obsolete: true
Created attachment 807020 [details] [diff] [review]
BorrowedContext support for cairo;
Attachment #807020 - Flags: review?(matt.woodrow)
Attachment #806945 - Attachment is obsolete: true
Attachment #806945 - Flags: review?(matt.woodrow)
Created attachment 807022 [details] [diff] [review]
Azure support in nsWindow;
Attachment #807022 - Flags: review?(matt.woodrow)
Attachment #806945 - Attachment is obsolete: false
Attachment #800440 - Attachment is obsolete: true
Created attachment 807023 [details] [diff] [review]
Azure content rendering on Linux;
Attachment #807023 - Flags: review?(roc)
Attachment #800438 - Attachment is obsolete: true
Attachment #800438 - Flags: review?(jmuizelaar)
Attachment #806945 - Attachment is obsolete: true
Created attachment 807024 [details] [diff] [review]
Enable content pref;
Attachment #807024 - Flags: review?(matt.woodrow)
Attachment #807023 - Flags: review?(roc) → review+
Comment on attachment 807024 [details] [diff] [review]
Enable content pref;

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

::: gfx/thebes/gfxPlatformGtk.cpp
@@ +91,5 @@
>      gCodepointsWithNoFonts = new gfxSparseBitSet();
>      UpdateFontList();
>  #endif
>      uint32_t canvasMask = (1 << BACKEND_CAIRO) | (1 << BACKEND_SKIA);
> +    uint32_t contentMask = (1 << BACKEND_CAIRO) | (1 << BACKEND_SKIA);

We don't support skia really, nsWindow can only create DrawTargetCairo.
Attachment #807024 - Flags: review?(matt.woodrow) → review+
Comment on attachment 807022 [details] [diff] [review]
Azure support in nsWindow;

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

::: widget/gtk2/nsWindow.cpp
@@ +2116,5 @@
> +        ctx = new gfxContext(gfxPlatform::GetPlatform()->
> +            CreateDrawTargetForSurface(surf, intSize));
> +    } else if (gfxPlatform::GetPlatform()->
> +                   SupportsAzureContentForType(BACKEND_SKIA) &&
> +               surf->GetType() != gfxASurface::SurfaceTypeImage) {

==
Attachment #807022 - Flags: review?(matt.woodrow) → review+
> 
> We don't support skia really, nsWindow can only create DrawTargetCairo.

I should read the patches in order. This is fine.
Attachment #807020 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3466e81d3a41
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b248c5a0dc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d871c0d61510
https://hg.mozilla.org/integration/mozilla-inbound/rev/439ff7e64d35
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc8bddfca5f1
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/25ee493a6e17 - seems to have gone pretty well elsewhere, so far, but it turned b2g builds a rather bright shade of ruby red.
https://hg.mozilla.org/integration/mozilla-inbound/rev/83080ccb5bf0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d70c5c90f077
https://hg.mozilla.org/integration/mozilla-inbound/rev/e649d78553ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/cebeca8df986
https://hg.mozilla.org/integration/mozilla-inbound/rev/d074ae862d16
https://hg.mozilla.org/mozilla-central/rev/83080ccb5bf0
https://hg.mozilla.org/mozilla-central/rev/d70c5c90f077
https://hg.mozilla.org/mozilla-central/rev/e649d78553ec
https://hg.mozilla.org/mozilla-central/rev/cebeca8df986
https://hg.mozilla.org/mozilla-central/rev/d074ae862d16
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 924679
Is Skia enabled by default? Why is this in the release notes?
Flags: needinfo?(ajones)
Azure content is now enabled by default. It uses the cairo backend. This bug actually gets both skia and cairo support working.
Flags: needinfo?(ajones)

Comment 44

4 years ago
After the upgrade to Firefox 27 (Linux x86_64), the defaults for me are:

gfx.canvas.azure.backends = cairo
gfx.content.azure.backends = cairo

Changing both to skia, makes the demo referenced above (http://js1k.com/2013-spring/demo/1555) to run much faster. Why is cairo used as the default then?

Updated

4 years ago
Depends on: 967763
The issue is that skia draws in process whereas cairo draws predominantly in the X server. The issue is that the gtk2 widgets live in the X server. If we use skia for content then we need to read back the gtk2 widgets from the X server to composite them with skia. This adds a big overhead to every composite cycle.

The net result is that regular browsing is slower with skia but benchmarks may show a faster result.

Comment 46

4 years ago
Is it any different with GTK3?
GTK3 lets us do widget (native theme) drawing in-process, so we would no longer need to pay the readback cost.

We still need to upload the final results back to the X server (either using XRender or OpenGL), which is an extra cost. I suspect the relative performance here will heavily depend on which GPU drivers you have installed (which decides how good your XRender implementation is).

Using SkiaGL to generate content directly onto the GPU should be a clearer win, but again, heavily driver dependent.

GTK3 is still blocked on finding a solution that won't break flash I believe.

Updated

3 years ago
Depends on: 1030383

Comment 48

a year ago
I just tested in with Firefox 46.0 (beta 11) which already uses gtk3. So far I don't see any issues when skia is used, and Flash is working normally as well.

Will skia be enabled by default in the release of 46.0?
You need to log in before you can comment on or make changes to this bug.