Bug 66154 – Ownership of canvas's GraphicsContext3D should be moved to PlatformContextSkia

• Component: New Bugs
• Status: RESOLVED FIXED

[Stephen White]

Ownership of canvas's GraphicsContext3D should be moved to PlatformContextSkia

[Stephen White]

Created attachment 103790 [details]
Patch

[Stephen White]

The purpose of this patch is to allow a PlatformContextSkia to "de-accelerate" itself if DrawingBuffer FBO creation fails.  This will allow us to delay FBO creation until the first drawing call (in a future patch).  To do this, the "ownership" of the GraphicsContext3D is moved into PlatformContextSkia (in fact, it doesn't own anything at all and remains a bare pointer, but one that can be set to NULL to indicate de-acceleration).

One major change it makes is that the GraphicsContext3D used for canvas2d is now a function-static, created when the first accelerated canvas is created, and is not destroyed until the renderer process exits.  This also means the GPU process will stay alive until that renderer exits as well.  This allows us to easily handle the case of canvas 2D's ImageBuffer (and GraphicsContext and PlatformContext) destruction and re-creation: we know the context will remain alive over reset, and won't incur a performance penalty.  I think this is acceptable, since as we move to more and more accelerated rendering, the GPU process will be hanging around more often anyway, but I welcome your feedback.

[Kenneth Russell]

Comment on attachment 103790 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:40
> +    static RefPtr<GraphicsContext3D> context = GraphicsContext3D::create(attributes, window);

What happens if the window that the GraphicsContext3D is initially created against goes away?

What happens if the GPU process crashes (context is lost)?

[James Robinson]

For offscreen contexts it doesn't matter what HostWindow is used to create the context and it doesn't matter if/when the HostWindow goes away.

I'm pretty sure we make no attempt to recover from a GPU process crash in the canvas path.

[Stephen White]

Comment on attachment 103790 [details]
Patch

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

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:40
>> +    static RefPtr<GraphicsContext3D> context = GraphicsContext3D::create(attributes, window);
> 
> What happens if the window that the GraphicsContext3D is initially created against goes away?
> 
> What happens if the GPU process crashes (context is lost)?

Good questions.

All canvases in the same rendering process currently use the same context (not new to this patch), which means the hostWindow is "wrong" for some of them.  Apparently, that is ok since the only thing it's used for at creation is to get the active URL for logging.  (Looking a bit further, it seems we also use it to determine of accelerated compositing is active.  I'm not sure if we ever get into a state where that WebViewImpl can be non-composited while the original one was, not sure, but also not new to this patch).

Canvas in general cannot handle context loss, and sets the appropriate flag in context creation, but I'm not sure if we should be doing something further.  I'll try nuking the GPU process before and after this patch to see if there's something I missed.

[Kenneth Russell]

Comment on attachment 103790 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:40
>>> +    static RefPtr<GraphicsContext3D> context = GraphicsContext3D::create(attributes, window);
>> 
>> What happens if the window that the GraphicsContext3D is initially created against goes away?
>> 
>> What happens if the GPU process crashes (context is lost)?
> 
> Good questions.
> 
> All canvases in the same rendering process currently use the same context (not new to this patch), which means the hostWindow is "wrong" for some of them.  Apparently, that is ok since the only thing it's used for at creation is to get the active URL for logging.  (Looking a bit further, it seems we also use it to determine of accelerated compositing is active.  I'm not sure if we ever get into a state where that WebViewImpl can be non-composited while the original one was, not sure, but also not new to this patch).
> 
> Canvas in general cannot handle context loss, and sets the appropriate flag in context creation, but I'm not sure if we should be doing something further.  I'll try nuking the GPU process before and after this patch to see if there's something I missed.

OK. Please give this a little stress testing but otherwise this sounds fine to me.

[Stephen White]

Just to verify:  killing the GPU process causes accelerated canvases to go blank, both before and after this patch.  Reloading the tab brings them back.

Ideally, it would be nice to drop to software in that case, but I that can wait for another patch.

[Stephen White]

Committed r93013: <http://trac.webkit.org/changeset/93013>