Tracking bug for Skia/Linux progress (content rendering)

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?

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

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

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?

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?

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.

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

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.

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.

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.

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.

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) { ==

> > We don't support skia really, nsWindow can only create DrawTargetCairo. I should read the patches in order. This is fine.

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

Is Skia enabled by default? Why is this in the release notes?

Azure content is now enabled by default. It uses the cairo backend. This bug actually gets both skia and cairo support working.

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?

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.

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?