Bug 86050 – [chromium] Move deferral-related logic out of Canvas2DLayerChromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Move deferral-related logic out of Canvas2DLayerChromium

[James Robinson]

Created attachment 141071 [details]
Patch

[Stephen White]

I'd really like junov@ to take a look at this, but unfortunately he's on vacation for a week.  Could you wait that long, or is this more pressing than that?

[James Robinson]

OK, but I'm pretty sure I'm not changing any logic (just relocating it).

[James Robinson]

Created attachment 141308 [details]
Patch

[James Robinson]

This just goes all the way over to using TextureLayerChromium for canvas 2d content.  It's a bigger patch, but I think it makes a lot more sense as one patch than two.  Happy to split it back out if reviewers would find that easier to manage.

[Stephen White]

Comment on attachment 141308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141308&action=review

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:110
> +SkCanvas* Canvas2DLayerBridge::skCanvas(SkDevice* device)

Maybe this should be createSkCanvas()?

Also, it's a bit muddled to have the canvas allocation happening here, but not ownership or deletion.  I think I'd prefer to have it back where it was, if possible, so all SkCanvases are allocated and freed in ImageBuffer and just set on this class.  That does seem to suggest that AcceleratedDeviceContext creation would move back too, though, which is perhaps against your intent.

Random thought:  I wonder if we could just make this class implement both the Client interface and the SkDeferredCanvas::DeviceContext interface as well?

> Source/WebCore/platform/graphics/chromium/ImageBufferDataSkia.h:38
> +class Canvas2DLayerBridge;

Not fond of the "Bridge" name, but I can't really come up with a better one either.

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:153
> +        const_cast<Canvas2DLayerBridge*>(m_data.m_layerBridge.get())->contextAcquired();

Eww.. (const_cast).

[Stephen White]

Comment on attachment 141308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141308&action=review

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:110
> +SkCanvas* Canvas2DLayerBridge::skCanvas(SkDevice* device)

Maybe this should be createSkCanvas()?

Also, it's a bit muddled to have the canvas allocation happening here, but not ownership or deletion.  I think I'd prefer to have it back where it was, if possible, so all SkCanvases are allocated and freed in ImageBuffer and just set on this class.  That does seem to suggest that AcceleratedDeviceContext creation would move back too, though, which is perhaps against your intent.

Random thought:  I wonder if we could just make this class implement both the Client interface and the SkDeferredCanvas::DeviceContext interface as well?

> Source/WebCore/platform/graphics/chromium/ImageBufferDataSkia.h:38
> +class Canvas2DLayerBridge;

Not fond of the "Bridge" name, but I can't really come up with a better one either.

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:153
> +        const_cast<Canvas2DLayerBridge*>(m_data.m_layerBridge.get())->contextAcquired();

Eww.. (const_cast).

[James Robinson]

Comment on attachment 141308 [details]
Patch

Good suggestions.  I'll update this patch with what I discover with those in mind.  Clearing review? until then.

[James Robinson]

Created attachment 142111 [details]
Patch

[James Robinson]

(In reply to comment #7)
> (From update of attachment 141308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141308&action=review
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:110
> > +SkCanvas* Canvas2DLayerBridge::skCanvas(SkDevice* device)
> 
> Maybe this should be createSkCanvas()?
> 
> Also, it's a bit muddled to have the canvas allocation happening here, but not ownership or deletion.  I think I'd prefer to have it back where it was, if possible, so all SkCanvases are allocated and freed in ImageBuffer and just set on this class.  That does seem to suggest that AcceleratedDeviceContext creation would move back too, though, which is perhaps against your intent.
> 
> Random thought:  I wonder if we could just make this class implement both the Client interface and the SkDeferredCanvas::DeviceContext interface as well?
> 
This appears to work and is fairly nice, except I believe it means I have to buy in to the SkRefCnt management for this class.  I think I'm doing it correctly in this patch (the tests pass and it doesn't crash) but I may be one unref() short - can you check that?

[Stephen White]

(In reply to comment #10)
> (In reply to comment #7)
> > (From update of attachment 141308 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=141308&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:110
> > > +SkCanvas* Canvas2DLayerBridge::skCanvas(SkDevice* device)
> > 
> > Maybe this should be createSkCanvas()?
> > 
> > Also, it's a bit muddled to have the canvas allocation happening here, but not ownership or deletion.  I think I'd prefer to have it back where it was, if possible, so all SkCanvases are allocated and freed in ImageBuffer and just set on this class.  That does seem to suggest that AcceleratedDeviceContext creation would move back too, though, which is perhaps against your intent.
> > 
> > Random thought:  I wonder if we could just make this class implement both the Client interface and the SkDeferredCanvas::DeviceContext interface as well?
> > 
> This appears to work and is fairly nice, except I believe it means I have to buy in to the SkRefCnt management for this class.  I think I'm doing it correctly in this patch (the tests pass and it doesn't crash) but I may be one unref() short - can you check that?

I'm not super-familiar with this idiom (internally, skia doesn't actually aggregate objects via SkAutoTUnref very much), but I think you're ok:  there's one ref on creation, but zero refs on SkAutoTUnref::reset().  One ref and one unref by the SkDeferredCanvas::DeferredDevice, and one unref in the SkAutoTUnref d'tor.

If you want to be certain, maybe you could check with some printf's in the Canvas2DLayerBridge destructor (or valgrind, if you're patient).

[Stephen White]

Comment on attachment 142111 [details]
Patch

Looks good (assuming it's been smoke-tested w/impl thread with and without deferral).  Thanks for the extra effort.  r=me

[James Robinson]

So there's one problem with making the bridge be an SkDeferredCanvas::DeviceContext in threaded deferred mode.  The basic issue is DeviceContext is refcounted and skia holds the last ref.  This means when an ImageBuffer is being destroyed, ganesh goes ahead and deletes the texture, but I've hooked up the logic to tell the compositor the texture is dead to ~Canvas2DLayerBridge() which isn't called until skia drops that ref.  Thus we might try to draw a frame with the texture that's already been deleted which makes the command buffer sad.

I think I'll have the DeviceContext be held by the Bridge, sort of how it was previous, by try to keep the SkCanvas creation in ImageBufferSkia.cpp.  Will update patch momentarily...

[James Robinson]

Created attachment 142597 [details]
Patch

[James Robinson]

Would you mind taking one more look, Stephen?  Changes are:

1.) Add back a private AcceleratedDeviceContext class since it's refcounted by skia
2.) Add a m_layer->clearClient() call in ~Canvas2DLayerBridge, since it is the client's layer.

These were found mostly by resizing FishIE a whole lot in debug/release with the 4 modes (thread on/off, deferral on/off).

[Stephen White]

(In reply to comment #15)
> Would you mind taking one more look, Stephen?  Changes are:
> 
> 1.) Add back a private AcceleratedDeviceContext class since it's refcounted by skia
> 2.) Add a m_layer->clearClient() call in ~Canvas2DLayerBridge, since it is the client's layer.
> 
> These were found mostly by resizing FishIE a whole lot in debug/release with the 4 modes (thread on/off, deferral on/off).

(In reply to comment #13)
> So there's one problem with making the bridge be an SkDeferredCanvas::DeviceContext in threaded deferred mode.  The basic issue is DeviceContext is refcounted and skia holds the last ref.  This means when an ImageBuffer is being destroyed, ganesh goes ahead and deletes the texture, but I've hooked up the logic to tell the compositor the texture is dead to ~Canvas2DLayerBridge() which isn't called until skia drops that ref.  Thus we might try to draw a frame with the texture that's already been deleted which makes the command buffer sad.

Could be fixed by setting the bridge's textureId to 0 in ImageBuffer[Skia]'s destructor?  Then the compositor could stop using it before anyone gets destroyed.

Another solution might be to add some kind of "your texture is about to die" notification to SkDeferredCanvas::DeviceContext, so the bridge could tell the compositor not to draw with it, even if the bridge lives on.  Since this would require a skia change, I'd go with the above for now.

> I think I'll have the DeviceContext be held by the Bridge, sort of how it was previous, by try to keep the SkCanvas creation in ImageBufferSkia.cpp.  Will update patch momentarily...

[James Robinson]

(In reply to comment #16)
> (In reply to comment #15)
> > Would you mind taking one more look, Stephen?  Changes are:
> > 
> > 1.) Add back a private AcceleratedDeviceContext class since it's refcounted by skia
> > 2.) Add a m_layer->clearClient() call in ~Canvas2DLayerBridge, since it is the client's layer.
> > 
> > These were found mostly by resizing FishIE a whole lot in debug/release with the 4 modes (thread on/off, deferral on/off).
> 
> (In reply to comment #13)
> > So there's one problem with making the bridge be an SkDeferredCanvas::DeviceContext in threaded deferred mode.  The basic issue is DeviceContext is refcounted and skia holds the last ref.  This means when an ImageBuffer is being destroyed, ganesh goes ahead and deletes the texture, but I've hooked up the logic to tell the compositor the texture is dead to ~Canvas2DLayerBridge() which isn't called until skia drops that ref.  Thus we might try to draw a frame with the texture that's already been deleted which makes the command buffer sad.
> 
> Could be fixed by setting the bridge's textureId to 0 in ImageBuffer[Skia]'s destructor?  Then the compositor could stop using it before anyone gets destroyed.

That works, but it'd require ImageBufferSkia reaching in to the bridge to tell it "You aren't going to be deleted just yet, but stop doing this".  I think it's clearer to have the bridge be OwnPtr<> by ImageBufferSkia and have the DeviceContext hang off of the bridge as the latest patch does.  Then the C++ object lifetimes map well to the resources they represent:

*) ImageBufferSkia has ownership of the SkCanvas, back buffer texture, and the Canvas2DLayerBridge.
*) Canvas2DLayerBridge has a reference to (but not ownership of) the back buffer texture and SkCanvas and has ownership of the front buffer texture if double buffering is enabled.  Creates and manages the layer.  If the canvas is deferred, Canvas2DLayerBridge has a ref to an SkDeferredCanvas::DeviceContext.
*) AcceleratedDeviceContext - lifetime managed by Canvas2DLayerBridge and Skia, has a ref to the layer and context.

The issue I have with having a death notification on the bridge is it puts it in a strange state where it's still alive but a lot of its member variables are dead (SkCanvas, front buffer texture).

[Stephen White]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Would you mind taking one more look, Stephen?  Changes are:
> > > 
> > > 1.) Add back a private AcceleratedDeviceContext class since it's refcounted by skia
> > > 2.) Add a m_layer->clearClient() call in ~Canvas2DLayerBridge, since it is the client's layer.
> > > 
> > > These were found mostly by resizing FishIE a whole lot in debug/release with the 4 modes (thread on/off, deferral on/off).
> > 
> > (In reply to comment #13)
> > > So there's one problem with making the bridge be an SkDeferredCanvas::DeviceContext in threaded deferred mode.  The basic issue is DeviceContext is refcounted and skia holds the last ref.  This means when an ImageBuffer is being destroyed, ganesh goes ahead and deletes the texture, but I've hooked up the logic to tell the compositor the texture is dead to ~Canvas2DLayerBridge() which isn't called until skia drops that ref.  Thus we might try to draw a frame with the texture that's already been deleted which makes the command buffer sad.
> > 
> > Could be fixed by setting the bridge's textureId to 0 in ImageBuffer[Skia]'s destructor?  Then the compositor could stop using it before anyone gets destroyed.
> 
> That works, but it'd require ImageBufferSkia reaching in to the bridge to tell it "You aren't going to be deleted just yet, but stop doing this".  I think it's clearer to have the bridge be OwnPtr<> by ImageBufferSkia and have the DeviceContext hang off of the bridge as the latest patch does.  Then the C++ object lifetimes map well to the resources they represent:
> 
> *) ImageBufferSkia has ownership of the SkCanvas, back buffer texture, and the Canvas2DLayerBridge.
> *) Canvas2DLayerBridge has a reference to (but not ownership of) the back buffer texture and SkCanvas and has ownership of the front buffer texture if double buffering is enabled.  Creates and manages the layer.  If the canvas is deferred, Canvas2DLayerBridge has a ref to an SkDeferredCanvas::DeviceContext.
> *) AcceleratedDeviceContext - lifetime managed by Canvas2DLayerBridge and Skia, has a ref to the layer and context.
> 
> The issue I have with having a death notification on the bridge is it puts it in a strange state where it's still alive but a lot of its member variables are dead (SkCanvas, front buffer texture).

OK, fair enough.  I still don't like the AcceleratedDeviceContext, but that's partly just my feeling that we have too damn many Contexts in the code already, and it's not your fault.

Perhaps we can fix up the refptr ownership of ADC in Skia and then revisit this design later.

[Stephen White]

Comment on attachment 142597 [details]
Patch

OK.  r=me

[Stephen White]

Comment on attachment 142597 [details]
Patch

Pulling my r+ for now, since junov@ has some comments he'd like to see addressed first.

[Justin Novosad]

Comment on attachment 142597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142597&action=review

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:47
> +    // This class derives from SkRefCnt (via SkDeferredCanvas::DeviceContext) and so should
> +    // always be retained in an SkAutoTUnref or similar (but not a WTF smart pointer type).

I think this comment no longer applies.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:71
> +    SkAutoTUnref<AcceleratedDeviceContext> m_deferredDeviceContext;

Is this ref count really necessary?  This object is already ref counted in SkDeferredCanvas::DeferredDevice.  I am pretty sure there is no need to even hold on to a reference to it in this class. Method deferredDeviceContext() could be replaced with createDeferredDeviceContext().

Something like:

Canvas2DLayerBridge::createDeferredDeviceContext() {
    ASSERT(m_deferralMode == Deferred);
    return new AcceleratedDeviceContext(m_context.get(), m_layer.get(), m_useDoubleBuffering)
}

[James Robinson]

Comment on attachment 142597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142597&action=review

I'll refresh this with the simplifications Justin suggested.

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:47
>> +    // always be retained in an SkAutoTUnref or similar (but not a WTF smart pointer type).
> 
> I think this comment no longer applies.

You're right, it does not

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:71
>> +    SkAutoTUnref<AcceleratedDeviceContext> m_deferredDeviceContext;
> 
> Is this ref count really necessary?  This object is already ref counted in SkDeferredCanvas::DeferredDevice.  I am pretty sure there is no need to even hold on to a reference to it in this class. Method deferredDeviceContext() could be replaced with createDeferredDeviceContext().
> 
> Something like:
> 
> Canvas2DLayerBridge::createDeferredDeviceContext() {
>     ASSERT(m_deferralMode == Deferred);
>     return new AcceleratedDeviceContext(m_context.get(), m_layer.get(), m_useDoubleBuffering)
> }

Hm - I think that would work, since ADC holds refs to everything it needs.  I'll do that instead. Thanks!

[James Robinson]

Created attachment 143682 [details]
Patch

[James Robinson]

This does just that - mind taking one more look, Stephen and/or Justin?

[Justin Novosad]

Me likes.

[Stephen White]

Comment on attachment 143682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143682&action=review

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:151
> +void Canvas2DLayerBridge::contextAcquired()

Sorry for more bikeshedding, but this name is ambiguous, given the plethora of contexts we have going on here (GraphicsContext3D, GraphicsContext, AcceleratedDeviceContext...).

Could we name it by what the function does (e.g., willModifyTexture()) rather than by what the caller is doing?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:154
> +    if (m_deferralMode == NonDeferred && !m_useDoubleBuffering)
> +        m_layer->willModifyTexture();

Just to verify:  this used to be protected by an impl-thread check.  Now it looks like it will be called even in the single-threaded case (where single-buffered, non-deferred is the default behaviour).  Is that because willModifyTexture is a no-op at the CCProxy level for the single-threaded case?

[James Robinson]

I had thought this landed weeks ago, but it appears I just dropped it.

(In reply to comment #26)
> (From update of attachment 143682 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143682&action=review
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:151
> > +void Canvas2DLayerBridge::contextAcquired()
> 
> Sorry for more bikeshedding, but this name is ambiguous, given the plethora of contexts we have going on here (GraphicsContext3D, GraphicsContext, AcceleratedDeviceContext...).
> 
> Could we name it by what the function does (e.g., willModifyTexture()) rather than by what the caller is doing?

willModifyTexture() is incorrect because we will only modify the texture here if we're in non-deferred mode - in deferred mode, we don't actually change anything until the prepareForDraw() call is made on the device context.  I think this name is about as good as we're going to get - if people are confused, they can look for callers.

> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:154
> > +    if (m_deferralMode == NonDeferred && !m_useDoubleBuffering)
> > +        m_layer->willModifyTexture();
> 
> Just to verify:  this used to be protected by an impl-thread check.  Now it looks like it will be called even in the single-threaded case (where single-buffered, non-deferred is the default behaviour).  Is that because willModifyTexture is a no-op at the CCProxy level for the single-threaded case?

Correct.  We're going to modify the texture in this instance so we need to notify the layer.  What that does inside the compositor implementation or inside the proxy is the compositor implementation's concern, not the bridge's concern.

[James Robinson]

Created attachment 145898 [details]
updated to ToT

[Stephen White]

(In reply to comment #27)
> I had thought this landed weeks ago, but it appears I just dropped it.
> 
> (In reply to comment #26)
> > (From update of attachment 143682 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143682&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:151
> > > +void Canvas2DLayerBridge::contextAcquired()
> > 
> > Sorry for more bikeshedding, but this name is ambiguous, given the plethora of contexts we have going on here (GraphicsContext3D, GraphicsContext, AcceleratedDeviceContext...).
> > 
> > Could we name it by what the function does (e.g., willModifyTexture()) rather than by what the caller is doing?
> 
> willModifyTexture() is incorrect because we will only modify the texture here if we're in non-deferred mode - in deferred mode, we don't actually change anything until the prepareForDraw() call is made on the device context.  I think this name is about as good as we're going to get - if people are confused, they can look for callers.

I don't think that's a good answer, since other people are going to be modifying this code once you're done with it.  Normally I'd say, take pity on them, and try to make the code self-documenting.

However, since deferral is now on by default and looks like it will stick, I think this code can go away soon anyway, so I'll let this go.

> > > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:154
> > > +    if (m_deferralMode == NonDeferred && !m_useDoubleBuffering)
> > > +        m_layer->willModifyTexture();
> > 
> > Just to verify:  this used to be protected by an impl-thread check.  Now it looks like it will be called even in the single-threaded case (where single-buffered, non-deferred is the default behaviour).  Is that because willModifyTexture is a no-op at the CCProxy level for the single-threaded case?
> 
> Correct.  We're going to modify the texture in this instance so we need to notify the layer.  What that does inside the compositor implementation or inside the proxy is the compositor implementation's concern, not the bridge's concern.

[Stephen White]

Comment on attachment 145898 [details]
updated to ToT

OK.  r=me

[James Robinson]

Committed r119769: <http://trac.webkit.org/changeset/119769>