Bug 80629 – [chromium] Canvas acceleration is not restored after context loss

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Stephen White]

1)  Load a page containing <canvas> in Chrome.
2)  Check that acceleration is active (e.g., FPS meter).
3)  Kill the GPU process.
4)  Reload the page.
5)  GPU acceleration is not restored.

See also http://crbug.com/117394.

[Stephen White]

Created attachment 130892 [details]
Patch

[Stephen White]

Created attachment 130910 [details]
Patch

[James Robinson]

Comment on attachment 130910 [details]
Patch

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

Would you mind (briefly) describing the semantics in SharedGraphicsContext3D.h? I believe the rules are something like: every caller can hold a ref for as long as they like to the returned context. they should periodically check to see if it's been lost. if the context has been lost, they should drop their ref and go ask SharedGraphicsContext3D for a new context. they may or may not get one. does that sound about right?

> Source/WebCore/ChangeLog:9
> +        Tested manually by killing the GPU process (no testing infrastructure
> +        to simulate lost context yet).

have you seen LayoutTests/platform/chromium/compositing/lost-context-* ?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:49
> +        if (m_context && !m_context->makeContextCurrent()) {

are you sure that if we lose a context we can't make it current? i think this may need to be separate conditions

if we do have a non-null context and we can't make it current, i don't think that means implies that getGraphicsResetStatusARB() is going to return something other than NO_ERROR - what if we can't allocate a command buffer at all?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:53
> +                // If we still can't make it current, just give up.

i think this function might be a little clearer if structured as:

if (m_context && it's lost or we can't make it current)
    m_context.clear();

if (!m_context)
  m_context = new thingy

return m_context.get()

[Stephen White]

(In reply to comment #3)
> (From update of attachment 130910 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130910&action=review
> 
> Would you mind (briefly) describing the semantics in SharedGraphicsContext3D.h? I believe the rules are something like: every caller can hold a ref for as long as they like to the returned context. they should periodically check to see if it's been lost. if the context has been lost, they should drop their ref and go ask SharedGraphicsContext3D for a new context. they may or may not get one. does that sound about right?

That sounds right.  I'll put that in a comment.

> > Source/WebCore/ChangeLog:9
> > +        Tested manually by killing the GPU process (no testing infrastructure
> > +        to simulate lost context yet).
> 
> have you seen LayoutTests/platform/chromium/compositing/lost-context-* ?

Unfortunately, it's compositor specific.  I tried plumbing through a similar loseCanvasContext() call in LayoutTestController, and it turned into kudzu that quickly spread to a ridiculous number of files.  I think the right thing to do something at a lower level, perhaps something like this:
https://chromiumcodereview.appspot.com/9609008/patch/19001/20007.

> 
> > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:49
> > +        if (m_context && !m_context->makeContextCurrent()) {
> 
> are you sure that if we lose a context we can't make it current? i think this may need to be separate conditions

When I did that, things went all crashy-crashy.  Not sure why...  I'll investigate further.

> if we do have a non-null context and we can't make it current, i don't think that means implies that getGraphicsResetStatusARB() is going to return something other than NO_ERROR - what if we can't allocate a command buffer at all?

I thought GraphicsContext3D::create() would return NULL in that case?

> > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:53
> > +                // If we still can't make it current, just give up.
> 
> i think this function might be a little clearer if structured as:
> 
> if (m_context && it's lost or we can't make it current)
>     m_context.clear();
> 
> if (!m_context)
>   m_context = new thingy
> 
> return m_context.get()

Better, but not quite right, since we still need to check if we can make it current the second time.  Let me riff on it..

[Stephen White]

Created attachment 130925 [details]
Patch

[James Robinson]

(In reply to comment #4)
> > if we do have a non-null context and we can't make it current, i don't think that means implies that getGraphicsResetStatusARB() is going to return something other than NO_ERROR - what if we can't allocate a command buffer at all?
> 
> I thought GraphicsContext3D::create() would return NULL in that case?

The initialization is in two phases.  The first phase tries to establish a renderer<->GPU IPC channel if it doesn't exist and returns NULL if this fails or if it doesn't like something about the attributes.  The second phase of initialization takes place on the first makeContextCurrent() call and it's what actually creates the GLContext, command buffer, etc.  The second phase also binds you to a particular thread.

GraphicsContext3D::create() -> WebGraphicsContext3DCommandBufferImpl::Initialize()
first makeContextCurrent() Call -> WebGraphicsContext3DCommandBufferImpl::MaybeInitializeGL()


If we can establish a GPU channel, but we can't actually spin up a command buffer (maybe we're out of virtual address space to map the command buffer in), then GraphicsContext3D::create will return something non-NULL and the first makeContextCurrent() will return false.

[Stephen White]

Comment on attachment 130925 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:38
> +    // The caller may ref on this pointer as long as they like.

Whoops, that grammar is weird on.  Fixing on...

[James Robinson]

Comment on attachment 130925 [details]
Patch

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

R=me but looking at the code I'm pretty sure you gotta flip the makeCurrent and extension query around or it'll crash. Try inducing a failure in MaybeInitializeGL() to test that theory out

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:40
> +        if (m_context && ((m_context->getExtensions()->getGraphicsResetStatusARB() != GraphicsContext3D::NO_ERROR) || !m_context->makeContextCurrent()))

nit: don't we have to make the context current before querying the extensions? i think these have to be flipped around. obviously if we can't make the context current we shouldn't try to look at its extensions bits either.

[Stephen White]

Created attachment 130926 [details]
Patch

[Stephen White]

Created attachment 130928 [details]
Patch

[James Robinson]

Comment on attachment 130928 [details]
Patch

R=me. Definitely want to kick the tires a bit manually to flush stuff out - maybe make every Nth context initialization fail in different places and kill the GPU process a whole bunch.

[Stephen White]

Created attachment 130935 [details]
Patch

[Stephen White]

Further manual testing (by forcing the return of NULL on the 2nd call to SharedGraphicsContext3D::get()) revealed that other parts of the code were not yet ready to handle this case.  In particular, checking for context loss in Canvas2DLayerChromium::drawsContext() was crashing (but sadly, not in Debug, so I'm not sure what's going on there -- the pointer should still be valid).  I've put in the same change as in WebGLLayerChromium:  check for context loss in Canvas2DLayerChromium::paintContentsIfDirty(), and check that flag in Canvas2DLayerChromium::setNeedsDisplayRect().

In order to make things clearer, I'm also going to return an actual PassRefPtr from SharedGraphicsContext3D::get(), to force the caller to evaluate ownership.  This required the patch to get a little bigger, as I audited the bare ptr usage throughout the code:

Canvas2DLayerChromium will own the ref used for canvas 2D.

PlatformContextSkia: removed the bare context ptr and replaced it with a bool, since that's how it was being used anyway.

AcceleratedDeviceContext will still own a bare ptr, since its lifetime is as long as the Canvas2DLayerChromium (and in fact, I think we can will rework this callback to hold only a C2DLC pointer, instead of GC3D, and perhaps be owned by C2DLC instead, to ensure lifetime -- Justin to follow up).

RateLimiter also holds a bare pointer, so we must ensure we call stopRateLimiter() it when the CLC2D goes away (the same is done in WebGL).

[Stephen White]

(In reply to comment #13)
> This required the patch to get a little bigger, as I audited the bare ptr usage throughout the code:

Note:  one place I wasn't sure about is the new call WebViewImpl::sharedGraphicsContext3D().  As I understand it, this will only be used to set up a texture for Pepper, so as long as the pointer isn't stored, a bare ptr is fine.  If it is, we should probably change this to be ref'ed too.

[Stephen White]

Created attachment 131350 [details]
Patch

[James Robinson]

(In reply to comment #14)
> (In reply to comment #13)
> > This required the patch to get a little bigger, as I audited the bare ptr usage throughout the code:
> 
> Note:  one place I wasn't sure about is the new call WebViewImpl::sharedGraphicsContext3D().  As I understand it, this will only be used to set up a texture for Pepper, so as long as the pointer isn't stored, a bare ptr is fine.

Correct - callers don't have ownership of this pointer and shouldn't be storing it.  It only needs to be valid for the callstack.

[James Robinson]

Comment on attachment 131350 [details]
Patch

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

Looks good!

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:228
> +    return CCRenderSurfaceFilters::apply(m_filters, m_contentsTexture->textureId(), m_contentRect.size(), SharedGraphicsContext3D::get().get());

get().get() - nice. need some more data().data().data() up in here

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:113
>      data->m_platformLayer = Canvas2DLayerChromium::create(context3D, size);

i think i'd find the ownership a bit easier to follow if this was a .release() call, just to make it clear that ownership of the local is being transferred to the Canvas2DLayerChromium and the layer was responsible for holding the context alive. i couldn't tell when initially looking at this why this function was taking a reference in a local and who was holding the ref after this function returned.

[Stephen White]

Comment on attachment 131350 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:113
>      data->m_platformLayer = Canvas2DLayerChromium::create(context3D, size);

Cool, I didn't know about RefPtr::release()!  Will fix on landing.

[Stephen White]

Committed r110483: <http://trac.webkit.org/changeset/110483>