Bug 65202 – Support moving GPU accelerated canvas to a new document (popup window)

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

[Tom Hudson]

Support moving GPU accelerated canvas to a new document (popup window)

[Tom Hudson]

Created attachment 102042 [details]
Patch

[Tom Hudson]

This is a proof-of-concept patch to solve http://code.google.com/p/chromium/issues/detail?id=86369. The issue is that a webpage that:
1. creates a canvas
2. creates a popup window
3. removes the canvas from the main window
4. adds the canvas to the popup window
did not work using the Chromium accelerated canvas.

The patch has two parts:
1. correctly reinitialize drawing when the canvas is added to a new document (and correspondingly shut down drawing when the canvas is removed from a document)
2. save the image when the canvas is removed from a document, and re-upload it to the graphics hardware when added to a new document

Known problems:
1. we're not sure how to test aside from manually
2. manual testing is fraught because the GPU renderer is frequently failing over into software

[Alok Priyadarshi]

You would need to sync with TOT. The file has changed since you last synced.

[James Robinson]

Comment on attachment 102042 [details]
Patch

I'm afraid this won't handle the case where a document is moved from one Page to another, since in that case nothing changes on the element level.

The only way I can think of to deal with this is to change the shared GraphicsContext3D to be per-process instead of per-Page.

[Alok Priyadarshi]

(In reply to comment #4)
> (From update of attachment 102042 [details])
> I'm afraid this won't handle the case where a document is moved from one Page to another, since in that case nothing changes on the element level.
> 

I believe this case could be handled by simply re-parenting the shared GraphicsContext3D. Al recently added support for re-parenting off-screen contexts. But it may not be exposed to WebKit yet.

[James Robinson]

There are potentially multiple canvases using the shared context, however. What do you do when a subset of those canvases move to a different compositor?

[Alok Priyadarshi]

(In reply to comment #6)
> There are potentially multiple canvases using the shared context, however. What do you do when a subset of those canvases move to a different compositor?

Yes. I was suggesting re-parenting only in the case when the whole document is moved to another page. When the document moves, don't all canvases move along as well? I am fuzzy on difference between WebKit's page and Javascript's document.

When individual canvases are moved, Tom's current approach would work.

BTW from resource consumption aspect, there may be benefits of having a shared GraphicsContext3D per process.

[James Robinson]

There can be multiple documents in the same compositor context, though.

one render process :: 1-many PageGroups
1 PageGroup :: 1-many Page
1 Page :: 1 WebViewImpl :: 1 LayerRendererChromium :: 1 Compositor context
1 Page :: 1 FrameTree
1 FrameTree :: 1-many Document
1 Document :: 1-many elements

[Tom Hudson]

Created attachment 102053 [details]
Patch

[Tom Hudson]

Merging with Alok's work from Friday/Saturday made this patch much simpler.

James, can you construct a repro case for the problem you're worrying about? Maybe then we can create a separate bug to track its solution?

Note: the test posted with the original Chromium bug is now invalid, because 100x100 canvases are generated in software. The canvas size must be increased to at least 128x128 to test hardware acceleration.

[Gyuyoung Kim]

Comment on attachment 102053 [details]
Patch

Attachment 102053 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9249546

[Early Warning System Bot]

Comment on attachment 102053 [details]
Patch

Attachment 102053 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9250480

[Tom Hudson]

Created attachment 102056 [details]
Patch

[James Robinson]

Created attachment 102059 [details]
test page for moving canvas to popup and moving iframe containing canvas to popup

[James Robinson]

Test case added that includes an iframe case.  I don't think it's a different bug, though, it's the same issue just in a different form.  Doing this at the per-document level just isn't gonna work, I'm afraid.

[WebKit Review Bot]

Comment on attachment 102056 [details]
Patch

Attachment 102056 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9249565

New failing tests:
fast/canvas/font-update.html

[Tom Hudson]

It looks like my initial set of patches is not the right approach; although I'm pretty confident that the expected png for fast/canvas/font-update.html is wrong, this patch breaks that test. The test implies that it's valid to draw to a context which doesn't have a parent, and that when the context is reattached to a parent those changes should be visible.

[James Robinson]

(In reply to comment #17)
> It looks like my initial set of patches is not the right approach; although I'm pretty confident that the expected png for fast/canvas/font-update.html is wrong, this patch breaks that test. The test implies that it's valid to draw to a context which doesn't have a parent, and that when the context is reattached to a parent those changes should be visible.

That is valid, and it's done quite a bit.  The context exists to whether the canvas is in the DOM or not and is persistent.

Here's another case that should work: you should be able to call ctx.drawImage(othercanvas, ...) when othercanvas is in the popup and ctx is for a canvas in the main page and vice versa.  I can whip up a test page real quick.

[Tom Hudson]

A really simple change of my patch (moving all of canvasWillBeRemoved into the start of canvasInsertedIntoDocument) seems to address both the original bug and the fast/canvas/font-update.html layout test failure.

[Tom Hudson]

Created attachment 102173 [details]
Patch

[Alok Priyadarshi]

Comment on attachment 102173 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2027
> +    RefPtr<ImageData> imageInPreviousContext = getImageData(0, 0, canvas()->width(), canvas()->height(), ec);

For this to succeed, the old context should still be valid. Can we assume this?

[James Robinson]

Created attachment 102184 [details]
test page with canvas->canvas draws from main doc to poup

load this page up, click "popup", "move canvas", then "draw from main document canvas to popup's canvas". you should see a yellow square in the popup's canvas

[Tom Hudson]

Alok suggested that I look into the 3D canvas case. It displays the same problem: if you move an experimental-webgl context from the main window to a popup, it shows up blank in the popup. Fixing that will be a bit more complicated.

[James Robinson]

Comment on attachment 102173 [details]
Patch

Yeah, the webgl case is another interesting situation.

I really think that doing this at the DOM level will not work and it's a bad intermediate state to go through.  There are many cases where this will fall over in ways that are completely inexplicable to the author.  I really think that to get this right you'll need to fix it at the sharedGraphicsContext3D level and make resource sharing Do The Right Thing under the scenes.

[Tom Hudson]

*** This bug has been marked as a duplicate of bug 65402 ***