Bug 61662 – [Chromium] putImageData and drawImage fail with accelerated 2d canvas

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Canvas

[Justin Novosad]

Created attachment 95210 [details]
Main.html

This is in reference to Chromium bug:
http://code.google.com/p/chromium/issues/detail?id=84165

Steps: 
Open attached file Main.html in Chromium, with accelerated 2d canvas enabled
Press the "compute" button
Result-> the canvases are empty

Expected result: images should appear in the canvases

[Justin Novosad]

Created attachment 95212 [details]
Patch

[Justin Novosad]

This first patch is probably not the right solution, but it fixes the bug and does not break any layout tests.

The problem with the this patch is that it calls updateCompositorResources too early. This slowish method will end up being called twice rather than once when the canvas is resized (once when width is changed, once when height is changed).  The lazy update scheme that is currently in place (flagged using setTextureChanged) does not work. It ends up performing the update too late, causing the render to fail.

Also, I will add a layout test.

[Justin Novosad]

vangelis, jamesr, enne, 
I need the opinion of people who know the accelerated compositor architecture well. Knowing that the above patch fixes the bug, perhaps you have better ideas of how the issue should be resolved.

[James Robinson]

As you suspect this isn't quite right.  Why do you need this call so early?  This won't result in just one extra call, it could result in many more than that.

[Justin Novosad]

(In reply to comment #4)
> As you suspect this isn't quite right.  Why do you need this call so early?  This won't result in just one extra call, it could result in many more than that.

I suspect that the right fix might be something else entirely.  For some odd reason the contents of the canvas' FBO are not going through to the compositor layer's texture unless the compositor layers's texture is reallocated earlier.  This problem only manifests itself when the canvas is resized after it has already been composited, then the subsequent composite shows a blank canvas layer.  

The brute-force fix in that patch makes sure that the compositor layer's texture is reallocated before the canvas is drawn to following a resize.  That is probably overkill.  The canvas' FBO was already being correctly resized before re-drawing to it, and that should be sufficient. I am not sure whether the real solution is to move-up the call to updateCompositorResourcesRecursive. Maybe all that is missing is a glFlush somewhere?  I'll do some more tinkering...

[James Robinson]

Comment on attachment 95212 [details]
Patch

updateCompositorResources() does stuff in the compositor context, but that should not effect anything.  It also does a flush() on the grContext and a flush() on the canvas' context.  I suspect that you need one of those to happen.

You also need to construct a test case for this to guard against future regressions.

[Justin Novosad]

Created attachment 95920 [details]
Patch

[Justin Novosad]

The latest patch I uploaded is still just a provisional patch.  I am posting for people to try it out.  This does not feel as icky as the previous one, but it still feels like fixing a TV by banging on it.  I am not satisfied with a fix that just works, it also needs to make sense...  So I have to ask: does this patch make sense to anyone?

[WebKit Review Bot]

Comment on attachment 95920 [details]
Patch

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

[James Robinson]

Comment on attachment 95920 [details]
Patch

Sorry, this doesn't compile on ToT and it doesn't really make sense.  The canvas context should be fairly independent of the compositor context, so flushing the compositor's context should have no effect.

I wonder if this is exposing some timing issue in the command buffer or in ganesh internals.  As frustrating as I know this is for you, we really need to have a full understanding of this issue to make sure that we are fixing the real root cause bug here and not just patching up one manifestation.

[James Robinson]

Created attachment 95945 [details]
offscreen canvas example

Here's a testcase using a completely offscreen canvas.  The canvas is never composited directly, instead its pixels are read back (using toDataURL) and set as the source of an <img> in the page.  On firefox or chrome w/ ganesh canvas disabled, there's a 50x50 image of random pixels.  With accelerated 2d canvas the image is blank (every pixel is 0x0000000).

[Justin Novosad]

In the end, it looks like this was a bug in Angle, not WebKit.  
The example posted by jamesr turned out to be a different bug that affected linux. The fix for that one was in Skia.
Nothing needs to be done in WebKit for this issue.
Bug Closed.