Bug 65202 - Support moving GPU accelerated canvas to a new document (popup window)
Summary: Support moving GPU accelerated canvas to a new document (popup window)
Status: RESOLVED DUPLICATE of bug 65402
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-26 12:52 PDT by Tom Hudson
Modified: 2011-08-24 11:35 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.51 KB, patch)
2011-07-26 12:56 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff
Patch (5.36 KB, patch)
2011-07-26 14:39 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2011-07-26 14:48 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff
test page for moving canvas to popup and moving iframe containing canvas to popup (1.13 KB, text/html)
2011-07-26 15:19 PDT, James Robinson
no flags Details
Patch (4.50 KB, patch)
2011-07-27 12:56 PDT, Tom Hudson
jamesr: review-
Details | Formatted Diff | Diff
test page with canvas->canvas draws from main doc to poup (1.52 KB, text/html)
2011-07-27 14:13 PDT, James Robinson
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Hudson 2011-07-26 12:52:58 PDT
Support moving GPU accelerated canvas to a new document (popup window)
Comment 1 Tom Hudson 2011-07-26 12:56:31 PDT
Created attachment 102042 [details]
Patch
Comment 2 Tom Hudson 2011-07-26 13:02:23 PDT
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
Comment 3 Alok Priyadarshi 2011-07-26 13:08:28 PDT
You would need to sync with TOT. The file has changed since you last synced.
Comment 4 James Robinson 2011-07-26 13:32:40 PDT
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.
Comment 5 Alok Priyadarshi 2011-07-26 13:35:49 PDT
(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.
Comment 6 James Robinson 2011-07-26 13:41:25 PDT
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?
Comment 7 Alok Priyadarshi 2011-07-26 13:47:28 PDT
(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.
Comment 8 James Robinson 2011-07-26 13:54:15 PDT
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
Comment 9 Tom Hudson 2011-07-26 14:39:10 PDT
Created attachment 102053 [details]
Patch
Comment 10 Tom Hudson 2011-07-26 14:42:49 PDT
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.
Comment 11 Gyuyoung Kim 2011-07-26 14:42:49 PDT
Comment on attachment 102053 [details]
Patch

Attachment 102053 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9249546
Comment 12 Early Warning System Bot 2011-07-26 14:44:46 PDT
Comment on attachment 102053 [details]
Patch

Attachment 102053 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9250480
Comment 13 Tom Hudson 2011-07-26 14:48:29 PDT
Created attachment 102056 [details]
Patch
Comment 14 James Robinson 2011-07-26 15:19:57 PDT
Created attachment 102059 [details]
test page for moving canvas to popup and moving iframe containing canvas to popup
Comment 15 James Robinson 2011-07-26 15:20:29 PDT
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.
Comment 16 WebKit Review Bot 2011-07-26 15:27:52 PDT
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
Comment 17 Tom Hudson 2011-07-27 07:32:45 PDT
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.
Comment 18 James Robinson 2011-07-27 11:37:23 PDT
(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.
Comment 19 Tom Hudson 2011-07-27 12:20:51 PDT
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.
Comment 20 Tom Hudson 2011-07-27 12:56:28 PDT
Created attachment 102173 [details]
Patch
Comment 21 Alok Priyadarshi 2011-07-27 13:04:16 PDT
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?
Comment 22 James Robinson 2011-07-27 14:13:44 PDT
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
Comment 23 Tom Hudson 2011-07-27 14:15:57 PDT
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.
Comment 24 James Robinson 2011-07-27 14:19:50 PDT
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.
Comment 25 Tom Hudson 2011-08-24 11:35:30 PDT

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