Closed Bug 740200 Opened 12 years ago Closed 11 years ago

Get Azure/Skia content rendering working on Linux

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: gw280, Assigned: ajones)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 13 obsolete files)

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)
Depends on: 740202
Depends on: 740580
Depends on: 740560
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)
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
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
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
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
Attachment #661105 - Attachment is obsolete: true
Attachment #665778 - Flags: review?(jmuizelaar)
Attachment #661106 - Attachment is obsolete: true
Attachment #665779 - Flags: review?(jmuizelaar)
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
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
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+
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
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
Attachment #665778 - Attachment is obsolete: true
Attachment #665779 - Attachment is obsolete: true
Attachment #665781 - Attachment is obsolete: true
Attachment #800435 - Flags: review?(jmuizelaar)
Assignee: gwright → ajones
Attachment #800438 - Flags: review?(jmuizelaar)
Attached patch Azure support in nsWindow; (obsolete) — Splinter Review
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-
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)
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+
Attachment #806945 - Flags: review?(matt.woodrow)
Attachment #806411 - Attachment is obsolete: true
Attachment #806411 - Flags: review?(jmuizelaar)
Attachment #661104 - Attachment is obsolete: true
Attachment #806945 - Attachment is obsolete: true
Attachment #806945 - Flags: review?(matt.woodrow)
Attachment #806945 - Attachment is obsolete: false
Attachment #800440 - Attachment is obsolete: true
Attachment #800438 - Attachment is obsolete: true
Attachment #800438 - Flags: review?(jmuizelaar)
Attachment #806945 - Attachment is obsolete: true
Attachment #807024 - Flags: review?(matt.woodrow)
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+
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.
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)
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?
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.
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.
Depends on: 1030383
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.