1395478 - Use capture DTs for box shadows

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement, P3)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Box shadows create manual draw targets and rasterize on the main thread, bypassing OMTP and creating performance cliffs. We need to fix this, especially since it could imply additional D2D locks.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, use CreateSimilarDrawTarget

This changes gfxBlur to use CreateSimilarDrawTarget, instead of Factory::CreateDrawTarget, so we don't have to add capturing hacks to this code. In a later patch we'll change DrawTargetCaptureImpl::CreateSimilarDrawTarget so it only creates capturing DTs.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, create capturing DTs in gfxBlur

Here, gfxBlur has a bunch of custom logic to decide what draw target backend to use. If the reference DT is capturing, we want the temporary surface to be capturing too.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 9, let capture DTs create new capture DTs

Right now a DrawTargetCapture will return a Direct2D/Skia target when CreateSimilarDrawTarget is called. This patch makes it so we always create new DrawTargetCaptures instead.

For temporary surfaces, the result is effectively the same. A call to Snapshot() will rasterize the temporary capture DT to the correct backend.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, CaptureCommandList abstraction

This extracts the DrawCommand storage logic out of DrawTargetCaptureImpl and into a separate CaptureCommandList class. This will allow us to re-use the command storage code in the next patch.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, add SourceSurfaceCapture

This patch adds a new surface type, CAPTURE, implemented by SourceSurfaceCapture. When DrawTargetCaptureImpl::Snapshot is called, we return one of these surfaces. The surface is basically a smaller DrawTargetCapture. It has a CaptureCommandList, a size, a SurfaceFormat, and a reference DT.

Snapshots are usually copy-on-write, and start out as a dumb reference to the DrawTarget that created them. If the original DrawTarget changes, the snapshot is given a temporary surface where rasterization is flushed, and then the snapshot is detached. If the snapshot is used before the DrawTarget changes, we don't have to create any temporary surfaces.

Unfortunately it is currently not possible to copy a DrawCommand, so we can't copy CaptureCommandLists, and we can't do copy-on-write for DrawTargetCapture snapshots. This patch instead *moves* the command list out of the DrawTargetCapture, and if copy-on-write is needed, the snapshot is marked as invalid. I don't know if this will work long-term, we might need to add copying as well.

Lastly, SourceSurfaceCapture has a Resolve() method that is basically the same as DrawTargetCapture::ExecuteOnDT. It also implements GetDataSurface as a fallback path. The D2D backend takes care to avoid this since it wants a SourceSurfaceD2D1.

With these patches we don't seem to create temporary DrawTargetD2D1s anymore, at least on a round of sites I tested. I did before and after measurements, and main thread time does seem to move to the paint thread. For example, on nu.nl, PaintOffMainThread time was 0.28ms on average, and we spent 0.0016ms waiting for paint flushes. After, PaintOffMainThread time was 0.18ms on average, and we spent 0.0089ms waiting for paint flushes. In theory this should help Skia too, though D2D needs it more due to lock contention.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

An easy fix for the copy-on-write problem: we could force immediate rasterization to a temporary surface, effectively calling Resolve() immediately. Then we don't need to hold a command list.

Hitting this case should be rare. We try to avoid the copy-on-write for temporary surfaces and don't usually observe snapshots while a DrawTarget is mutating.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8904850 [details] [diff] [review]
part 5, add SourceSurfaceCapture

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

::: gfx/2d/DrawTargetCapture.cpp
@@ +61,5 @@
>  
>  already_AddRefed<SourceSurface>
>  DrawTargetCaptureImpl::Snapshot()
>  {
> +  if (!mSnapshot) {

Can you add the comment attached to this patch in here?

@@ +326,5 @@
>  
>  already_AddRefed<DrawTarget>
>  DrawTargetCaptureImpl::CreateSimilarDrawTarget(const IntSize &aSize, SurfaceFormat aFormat) const
>  {
> +  printf_stderr("Creating similar DT\n");

nit: Delete

::: gfx/2d/DrawTargetD2D1.cpp
@@ +1914,5 @@
>                                     bool aUserSpace)
>  {
>    RefPtr<ID2D1Image> image;
>    switch (aSurface->GetType()) {
> +  case SurfaceType::CAPTURE:

I think we need something like this in DrawTargetSkia::GetSkImageForSurface as well.

@@ +1980,5 @@
> +    if (!resolved) {
> +      return nullptr;
> +    }
> +    MOZ_ASSERT(resolved->GetType() != SurfaceType::CAPTURE);
> +    return OptimizeSourceSurface(resolved);

Do we need to do this? Can't we pass the backend type to Resolve so that we automatically already draw the SourceSurfaceCapture with D2D1 instead of the refDT? Might not matter since OptimizeSourceSurface should be a noopt, but are we doing lots of conversions here when we shouldn't have to?

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8904836 [details] [diff] [review]
part 2, create capturing DTs in gfxBlur

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

Does this mean we're losing this logic about deciding if we should use acceleration or not?

We pass the desired backend type to CreateCaptureDrawTarget, can we pass something to force it to use software if that's what we decided we want?

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Mason Chang [:mchang] from comment #7)
> ::: gfx/2d/DrawTargetD2D1.cpp
> @@ +1914,5 @@
> >                                     bool aUserSpace)
> >  {
> >    RefPtr<ID2D1Image> image;
> >    switch (aSurface->GetType()) {
> > +  case SurfaceType::CAPTURE:
> 
> I think we need something like this in DrawTargetSkia::GetSkImageForSurface
> as well.

Initially I didn't do this since GetDataSurface should work, but I think that might make a copy, which we don't want. I'll add a fast-path.

> @@ +1980,5 @@
> > +    if (!resolved) {
> > +      return nullptr;
> > +    }
> > +    MOZ_ASSERT(resolved->GetType() != SurfaceType::CAPTURE);
> > +    return OptimizeSourceSurface(resolved);
> 
> Do we need to do this? Can't we pass the backend type to Resolve so that we
> automatically already draw the SourceSurfaceCapture with D2D1 instead of the
> refDT? Might not matter since OptimizeSourceSurface should be a noopt, but
> are we doing lots of conversions here when we shouldn't have to?

Yeah, that sounds like a reasonable thing to do.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Mason Chang [:mchang] from comment #7)
> ::: gfx/2d/DrawTargetCapture.cpp
> @@ +61,5 @@
> >  
> >  already_AddRefed<SourceSurface>
> >  DrawTargetCaptureImpl::Snapshot()
> >  {
> > +  if (!mSnapshot) {
> 
> Can you add the comment attached to this patch in here?

Which comment are you referring to?

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, add SourceSurfaceCapture v2

w/ comments addressed. I also added support for DrawTargetWillChange(), by immediately performing rasterization.

I would have liked to add some thread assertions here but it's tricky given that moz2d doesn't like to include Gecko.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, create capturing DTs in gfxBlur

Good catch, I think this should fix it.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, add SourceSurfaceCapture v2

Actually pass the desired backend to Resolve().

Comment 14

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8906182 [details] [diff] [review]
part 5, add SourceSurfaceCapture v2

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +1975,5 @@
>  
> +  // Special case captures so we don't resolve them to a data surface.
> +  if (aSurface->GetType() == SurfaceType::CAPTURE) {
> +    SourceSurfaceCapture* capture = static_cast<SourceSurfaceCapture*>(aSurface);
> +    RefPtr<SourceSurface> resolved = capture->Resolve();

We don't need to pass in the backend type here as well?

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7, implement DrawSurfaceWithShadow

Comment 16

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8907758 [details] [diff] [review]
part 7, implement DrawSurfaceWithShadow

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

::: gfx/2d/SourceSurfaceCapture.cpp
@@ +51,5 @@
>    // We don't want to rasterize these on the main thread, if we can avoid it.
>    // If we are called on the main thread, we don't cache the results and don't
>    // require the DrawTarget to be detached.
>    if (NS_IsMainThread()) {
> +    MOZ_CRASH("NYI");

Was this intended?

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Nope, thanks!

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7, fix bug & threadsafe assertions

This was a super subtle bug. SourceSurfaceCapture sometimes has to materialize an actual surface on the main thread. To avoid races with the paint thread, it didn't cache these surfaces. However, DrawTarget::DrawSurface expects callers to own a reference to the input surface.

What would happen is the SourceSurfaceCapture would be resolved to a temporary image, we'd pull the SkImage out, and then the resolved SourceSurfaceSkia would be destroyed along with the bits the SkImage was using.

This patch changes Resolve() to always cache the resolved image, so it functions properly with Moz2D.

It makes me a little nervous to do this since if two threads use the same SourceSurfaceCapture, it will implode. However there's really no reason to use locks here since having multiple threads own a Moz2D object is not legal.

Instead, this patch introduces a little helper class that debug-asserts when two threads try to mutate the same SourceSurfaceCapture. After OMTP has been law of the land for a while I'll feel comfortable tearing this out.

Comment 19

[Ryan Hunt [:rhunt]]

Comment on attachment 8912463 [details] [diff] [review]
part 7, fix bug & threadsafe assertions

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

::: gfx/2d/SourceSurfaceCapture.h
@@ +7,5 @@
>  #define mozilla_gfx_2d_SourceSurfaceCapture_h
>  
>  #include "2D.h"
>  #include "CaptureCommandList.h"
> +#include "mozilla/Mutex.h"

I don't think this is needed.

::: gfx/2d/ThreadsafeAssertions.h
@@ +11,5 @@
> +namespace mozilla {
> +namespace gfx {
> +
> +#if defined(DEBUG)
> +class MOZ_STACK_CLASS SingleThreadOwner

Is SourceSurfaceCapture only allowed on the stack? I think you may have meant this for AutoEnter.

@@ +18,5 @@
> +  SingleThreadOwner()
> +   : mOwner(0)
> +  {}
> +
> +  class AutoEnter

I think this should be MOZ_RAII.

@@ +37,5 @@
> +private:
> +  static intptr_t GetCurrentThread();
> +
> +  void Enter() {
> +    bool ok = mOwner.compareExchange(0, GetCurrentThread());

I think we could replace GetCurrentThread() with "1" and have the same behavior.

It'd be nice to know which thread is currently mutating the object if this assertion is ever triggered, so I'm fine with leaving it if we log it somehow in the assert.

@@ +41,5 @@
> +    bool ok = mOwner.compareExchange(0, GetCurrentThread());
> +    MOZ_RELEASE_ASSERT(ok, "No other threads may own this object");
> +  }
> +  void Leave() {
> +    bool ok = mOwner.compareExchange(mOwner, 0);

Is doing a mOwner.compareExchange(mOwner) necessary? Could we just atomically assign?

We should always be the current thread if we are reaching Leave().

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thanks, I've integrated those changes locally. This is still not ready to land as a single reftest is failing (bugs/1174332-1.html). Once that's solved this should be good to go.

Comment 21

[Ryan Hunt [:rhunt]]

Oh I forgot to add that it may be worth filing a bug for removing "SingleThreadOwner" in the future, so it isn't lost track of.

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7, fix skia interaction

This is a better version of part 7. Instead of adding threadsafe assertions, it just makes the class threadsafe. We had to do this for SourceSurfaceSkia so we might as well do it here.

That we have to keep the ref alive is kind of gross, but I couldn't see a way to fix it without a lot of refactoring. The D2D backend has an internal solution of keeping refs to resources alive until the next Flush(). But this should hold for now.

Comment 23

[Ryan Hunt [:rhunt]]

(In reply to David Anderson [:dvander] from comment #22)
> Created attachment 8919159 [details] [diff] [review]
> part 7, fix skia interaction
> 
> This is a better version of part 7. Instead of adding threadsafe assertions,
> it just makes the class threadsafe. We had to do this for SourceSurfaceSkia
> so we might as well do it here.
> 
> That we have to keep the ref alive is kind of gross, but I couldn't see a
> way to fix it without a lot of refactoring. The D2D backend has an internal
> solution of keeping refs to resources alive until the next Flush(). But this
> should hold for now.

I think you uploaded the wrong patch for this.

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7, fix skia interaction

Thanks, here's the right one.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, DrawTargetCapture for data

Taking a new approach, starting from getting blurs working and then optimizing all the edge cases.

This first patch adds Factory::CreateCaptureDrawTargetForData(). When the capture is resolved via Snapshot(), this will override the backend choice and force the given stride/surface size.

This will make it easier to guarantee in-place blur operations.

Comment 26

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, in-place BlurCommand

This adds a Blur function to DrawTarget, to perform an in-place blur. It is only supported on Skia right now. The idea is that, when gfxAlphaBoxBlur wants to blur on an unaccelerated surface, it can call this function whether or not the backend is a capture DT.

I don't see anything obvious that would indicate AlphaBoxBlur is thread-unsafe, so the object is just copied into the draw command.

Comment 27

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, support OMTP in gfxBlur

This patch changes gfxAlphaBoxBlur so that a DrawTargetCapture is created if the reference DT is capturing. This differs from the original patch in that we don't use CreateSimilarDrawTarget, and that the final blur operation is included in the command stream.

Note that in the accelerated case in DoBlur(), we will force main-thread rasterization of any capturing DT. This will be fixed in a separate patch.

Comment 28

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, clone commands

This adds the ability to clone DrawCommands into a new CommandList. Otherwise, a snapshot of a DrawTargetCapture will have to perform immediate rasterization if the draw target changes.

Comment 29

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6, SourceSurfaceCapture

This is all of the SourceSurfaceCapture patches rolled up, now with command list cloning.

Comment 30

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 8, MarkChanged optimizations

Sometimes we call SetTransform or PopClip on a DrawTarget that has an outstanding snapshot, even though the DrawTarget is about to go away. These operations don't actually affect underlying pixels so we shouldn't have to detach & copy the snapshot.

This patch adds a hint to each DrawCommand type so DrawTargetCapture can skip the MarkChanged call if unnecessary.

Comment 31

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, support OMTP in gfxBlur

This version fixes the accelerated case to use CreateSimilarDrawTarget. Factory::CreateDrawTarget() doesn't support BackendType::CAPTURE.

Comment 32

[Ryan Hunt [:rhunt]]

Comment on attachment 8921764 [details] [diff] [review]
part 2, in-place BlurCommand

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +2166,5 @@
>  
> +void
> +DrawTargetSkia::Blur(const AlphaBoxBlur& aBlur)
> +{
> +  Flush();

Do we need to MarkChanged() here?

Comment 33

[Ryan Hunt [:rhunt]]

Comment on attachment 8922016 [details] [diff] [review]
part 8, MarkChanged optimizations

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

Nice.

::: gfx/2d/DrawCommand.h
@@ +860,5 @@
>    {
>      aDT->SetPermitSubpixelAA(mPermitSubpixelAA);
>    }
>  
> +  static const bool AffectsSnapshot = true;

I don't see any implementations that MarkChanged on SetPermitSubpixelAA, so I think this should be false.

Comment 34

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Ryan Hunt [:rhunt] from comment #32)
> Comment on attachment 8921764 [details] [diff] [review]
> part 2, in-place BlurCommand
> 
> Review of attachment 8921764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +2166,5 @@
> >  
> > +void
> > +DrawTargetSkia::Blur(const AlphaBoxBlur& aBlur)
> > +{
> > +  Flush();
> 
> Do we need to MarkChanged() here?

Yup, thanks.

Comment 35

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 10, don't cache DrawTargetCaptures

Part 1-9 bounced on try. SourceSurfaceCapture asserts that the snapshot, if used on the paint thread, is detached from the owning DrawTarget. Other snapshot types don't have an assertion like this, but I wanted to protect against possible races with the main thread. We don't want the main thread detaching a snapshot as the paint thread reads it.

The assert fired because nsCanvasFrame calls CreateSimilarDrawTarget - which now returns a capturing DT - and caches it. On one hand, I don't think the cache is useless in this scenario because SourceSurfaceCapture caches its resolved pixels. We could consider just synchronizing the detach operation with the mutex SourceSurfaceCapture already has.

On the other hand, we can also just preserve the existing behavior, which is what this patch does. I'm okay with going either route.

I did audit all other callers of CreateSimilarDrawTarget and couldn't see any other cache-y callers, but I have another try run going to see.

Comment 36

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8923596 [details] [diff] [review]
part 10, don't cache DrawTargetCaptures

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

This seems easier and safer than trying to cache the resolved pixels for the SourceSurfaceCapture to me.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/491939c123ec
Add a factory method for creating data-backed DrawTargetCaptures. (bug 1395478 part 1, r=rhunt)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7488658f6a
Add a Blur DrawCommand. (bug 1395478 part 2, r=rhunt)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2fccfb3f00
Create DrawTargetCaptures for blur operations when OMTP is enabled. (bug 1395478 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/93d6f274e108
Create a CaptureCommandList abstraction for DrawTargetCapture. (bug 1395478 part 4, r=mchang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad3fd7b113d
Allow cloning draw commands. (bug 1395478 part 5, r=rhunt)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb9803a7a92
Add SourceSurfaceCapture to avoid main-thread rasterization with temporary DrawTargetCaptures. (bug 1395478 part 6, r=rhunt)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec65ee860e79
Implement DrawTargetCapture::DrawSurfaceWithShadow. (bug 1395478 part 7, r=mchang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/811d8f83793a
Don't call MarkChanged for commands that don't affect pixels. (bug 1395478 part 8, r=rhunt)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8280184b5ab7
When similar draw targets are requested from capturing DTs, always return a new capturing DT instead of an actual surface. (bug 1395478 part 9, r=mchang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/268aa3afa9e0
Don't cache DrawTargetCaptures in nsCanvasFrame. (bug 1395478 part 10, r=mattwoodrow)

Comment 38

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/491939c123ec
https://hg.mozilla.org/mozilla-central/rev/0f7488658f6a
https://hg.mozilla.org/mozilla-central/rev/6e2fccfb3f00
https://hg.mozilla.org/mozilla-central/rev/93d6f274e108
https://hg.mozilla.org/mozilla-central/rev/fad3fd7b113d
https://hg.mozilla.org/mozilla-central/rev/ccb9803a7a92
https://hg.mozilla.org/mozilla-central/rev/ec65ee860e79
https://hg.mozilla.org/mozilla-central/rev/811d8f83793a
https://hg.mozilla.org/mozilla-central/rev/8280184b5ab7
https://hg.mozilla.org/mozilla-central/rev/268aa3afa9e0