Use capture DTs for box shadows

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

40 Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(10 attachments, 10 obsolete attachments)

1.89 KB, patch
mchang
: review+
Details | Diff | Splinter Review
8.68 KB, patch
mchang
: review+
Details | Diff | Splinter Review
5.24 KB, patch
mchang
: review+
Details | Diff | Splinter Review
7.38 KB, patch
rhunt
: review+
Details | Diff | Splinter Review
15.58 KB, patch
rhunt
: review+
Details | Diff | Splinter Review
13.39 KB, patch
rhunt
: review+
Details | Diff | Splinter Review
19.72 KB, patch
rhunt
: review+
Details | Diff | Splinter Review
10.85 KB, patch
rhunt
: review+
Details | Diff | Splinter Review
4.14 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.38 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8904835 - Flags: review?(matt.woodrow)
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.
Attachment #8904836 - Flags: review?(matt.woodrow)
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.
Attachment #8904837 - Flags: review?(mchang)
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.
Attachment #8904839 - Flags: review?(mchang)
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.
Attachment #8904850 - Flags: review?(mchang)
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.
Attachment #8904837 - Flags: review?(mchang) → review+
Attachment #8904839 - Flags: review?(mchang) → review+
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?
Attachment #8904835 - Flags: review?(matt.woodrow) → review+
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?
(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.
(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?
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.
Attachment #8904850 - Attachment is obsolete: true
Attachment #8904850 - Flags: review?(mchang)
Attachment #8906077 - Flags: review?(mchang)
Priority: -- → P3
Whiteboard: [gfx-noted]
Good catch, I think this should fix it.
Attachment #8904836 - Attachment is obsolete: true
Attachment #8904836 - Flags: review?(matt.woodrow)
Attachment #8906180 - Flags: review?(matt.woodrow)
Actually pass the desired backend to Resolve().
Attachment #8906077 - Attachment is obsolete: true
Attachment #8906077 - Flags: review?(mchang)
Attachment #8906182 - Flags: review?(mchang)
Attachment #8906180 - Flags: review?(matt.woodrow) → review+
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?
Attachment #8906182 - Flags: review?(mchang) → review+
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?
Attachment #8907758 - Flags: review?(mchang) → review+
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.
Attachment #8912463 - Flags: review?(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().
Attachment #8912463 - Flags: review?(rhunt) → review+
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.
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.
Posted patch part 7, fix skia interaction (obsolete) — Splinter Review
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.
Attachment #8912463 - Attachment is obsolete: true
Attachment #8919159 - Flags: review?(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.
Posted patch part 7, fix skia interaction (obsolete) — Splinter Review
Thanks, here's the right one.
Attachment #8919159 - Attachment is obsolete: true
Attachment #8919159 - Flags: review?(rhunt)
Attachment #8919453 - Flags: review?(rhunt)

Updated

2 years ago
Attachment #8919453 - Flags: review?(rhunt) → review+
Attachment #8904835 - Attachment is obsolete: true
Attachment #8906180 - Attachment is obsolete: true
Attachment #8904837 - Attachment is obsolete: true
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.
Attachment #8921762 - Flags: review?(rhunt)
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.
Attachment #8921764 - Flags: review?(rhunt)
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.
Attachment #8921768 - Flags: review?(matt.woodrow)
Attachment #8906182 - Attachment is obsolete: true
Attachment #8919453 - Attachment is obsolete: true
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.
Attachment #8921783 - Flags: review?(rhunt)
This is all of the SourceSurfaceCapture patches rolled up, now with command list cloning.
Attachment #8921784 - Flags: review?(rhunt)
Attachment #8907758 - Attachment description: part 6, implement DrawSurfaceWithShadow → part 7, implement DrawSurfaceWithShadow
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.
Attachment #8922016 - Flags: review?(rhunt)
This version fixes the accelerated case to use CreateSimilarDrawTarget. Factory::CreateDrawTarget() doesn't support BackendType::CAPTURE.
Attachment #8921768 - Attachment is obsolete: true
Attachment #8921768 - Flags: review?(matt.woodrow)
Attachment #8922044 - Flags: review?(matt.woodrow)
Attachment #8904837 - Attachment description: part 3, let capture DTs create new capture DTs → part 9, let capture DTs create new capture DTs
Attachment #8904837 - Attachment is obsolete: false
Attachment #8922044 - Flags: review?(matt.woodrow) → review+

Updated

2 years ago
Attachment #8921762 - Flags: review?(rhunt) → review+
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?
Attachment #8921764 - Flags: review?(rhunt) → review+

Updated

2 years ago
Attachment #8921783 - Flags: review?(rhunt) → review+

Updated

2 years ago
Attachment #8921784 - Flags: review?(rhunt) → review+
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.
Attachment #8922016 - Flags: review?(rhunt) → review+
(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.
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.
Attachment #8923596 - Flags: review?(matt.woodrow)
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.
Attachment #8923596 - Flags: review?(matt.woodrow) → review+

Comment 37

2 years ago
Pushed by danderson@mozilla.com:
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)
Depends on: 1413862

Updated

Last year
Depends on: 1440937

Updated

Last year
Depends on: 1445481
Depends on: 1450814

Updated

Last year
Depends on: 1455930
You need to log in before you can comment on or make changes to this bug.