Bug 110003 - Printing WebGL canvases in Chrome uses stale data after first print
Summary: Printing WebGL canvases in Chrome uses stale data after first print
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brandon Jones
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-15 22:31 PST by Brandon Jones
Modified: 2013-02-20 17:36 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.55 KB, patch)
2013-02-15 22:33 PST, Brandon Jones
no flags Details | Formatted Diff | Diff
Test case suggested by jbauman (1.27 KB, text/html)
2013-02-19 12:10 PST, Kenneth Russell
no flags Details
Patch (1.60 KB, patch)
2013-02-20 10:42 PST, Brandon Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brandon Jones 2013-02-15 22:31:29 PST
Printing WebGL canvases in Chrome uses stale data after first print
Comment 1 Brandon Jones 2013-02-15 22:33:13 PST
Created attachment 188692 [details]
Patch
Comment 2 John Bauman 2013-02-15 22:54:39 PST
Comment on attachment 188692 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:743
> +        canvas()->clearPresentationCopy();

I suppose one place this breaks down is when in the course of one javascript sequence you draw to the canvas, do window.print, do a readpixels from it, then do window.print again. Although that case might already be broken.

This would also be broken if anyone ever used WebGL with compositing disabled, though that never actually happens.

It might make more sense to do the canvas()->clearPresentationCopy() in WebGLRenderingContext::markContextChanged, and not in this method.
Comment 3 Brandon Jones 2013-02-16 11:00:41 PST
(In reply to comment #2)
> (From update of attachment 188692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188692&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:743
> > +        canvas()->clearPresentationCopy();
> 
> I suppose one place this breaks down is when in the course of one javascript sequence you draw to the canvas, do window.print, do a readpixels from it, then do window.print again. Although that case might already be broken.

That's an interesting edge case. It would be good to know what the current behavior of this is, and if this patch changes it.

> This would also be broken if anyone ever used WebGL with compositing disabled, though that never actually happens.

Ken Russell and I talked about this, and we agree that it never happens in practice. We are discussing disabling WebGL context creation if accelerated compositing is off.

> It might make more sense to do the canvas()->clearPresentationCopy() in WebGLRenderingContext::markContextChanged, and not in this method.

paintRenderingResultsToCanvas is only called when printing, where a little extra overhead won't make much difference. markContextChanged is called frequently while drawing. Even though clearPresentationCopy is not an expensive call, I'd prefer not to add unnecessary calls to the main draw loop simply for the sake of printing.
Comment 4 Kenneth Russell 2013-02-19 12:10:30 PST
Created attachment 189136 [details]
Test case suggested by jbauman

@jbauman: here's the test case you suggested. In both Chrome Canary and the current WebKit nightly, the readPixels results contain (0, 0, 0, 0) after the window.print call, so clearly that's causing a re-composite and blowing away of the back buffer which isn't spec compliant. In Chrome, the second printing result contains red instead of green, although WebKit correctly contains green.

Brandon, can you try this test case with your patch? If it fixes Chrome's stale printing result without regressing anything else, then I am in favor of getting this fix in and proceeding with the other bugs afterward.
Comment 5 Brandon Jones 2013-02-19 12:57:43 PST
(In reply to comment #4)
> Created an attachment (id=189136) [details]
> Test case suggested by jbauman
> 
> @jbauman: here's the test case you suggested. In both Chrome Canary and the current WebKit nightly, the readPixels results contain (0, 0, 0, 0) after the window.print call, so clearly that's causing a re-composite and blowing away of the back buffer which isn't spec compliant. In Chrome, the second printing result contains red instead of green, although WebKit correctly contains green.
> 
> Brandon, can you try this test case with your patch? If it fixes Chrome's stale printing result without regressing anything else, then I am in favor of getting this fix in and proceeding with the other bugs afterward.

Just tested: The behavior of the test case is identical with or without the patch.
Comment 6 Kenneth Russell 2013-02-19 14:00:27 PST
Comment on attachment 188692 [details]
Patch

Thanks for testing. r=me
Comment 7 WebKit Review Bot 2013-02-19 16:20:29 PST
Comment on attachment 188692 [details]
Patch

Rejecting attachment 188692 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 188692, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 13671 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
53>At revision 13671.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/16640023
Comment 8 Brandon Jones 2013-02-20 10:42:04 PST
Created attachment 189337 [details]
Patch
Comment 9 WebKit Review Bot 2013-02-20 11:14:47 PST
Comment on attachment 189337 [details]
Patch

Rejecting attachment 189337 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189337, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
toinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 110] Connection timed out>

Full output: http://queues.webkit.org/results/16659436
Comment 10 WebKit Review Bot 2013-02-20 17:36:44 PST
Comment on attachment 189337 [details]
Patch

Clearing flags on attachment: 189337

Committed r143545: <http://trac.webkit.org/changeset/143545>
Comment 11 WebKit Review Bot 2013-02-20 17:36:47 PST
All reviewed patches have been landed.  Closing bug.