Bug 54561 - [chromium] Layout Test fast/canvas/setWidthResetAfterForcedRender.html fails on accelerated 2D canvas
Summary: [chromium] Layout Test fast/canvas/setWidthResetAfterForcedRender.html fails ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-16 08:48 PST by Stephen White
Modified: 2011-02-28 12:45 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.66 KB, patch)
2011-02-16 11:53 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (2.05 KB, patch)
2011-02-16 15:22 PST, Stephen White
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2011-02-16 08:48:25 PST
This layout test should produce a blank rectangle below the text, but produces a green one instead.  It seems that the accelerated path is not clearing the canvas as it should.instead
Comment 1 Stephen White 2011-02-16 10:46:11 PST
Notes for posterity:

This seems to be due to an interaction between the compositor and the accelerated 2D canvas.  If I run DumpRenderTree on this test with only --enable-accelerated-2d-canvas, it gives the correct result.  It's only when both that flag and --enable-accelerated-compositing are enabled that it fails (green square instead of white).

It seems as if the compositor is not updating the canvas after the DrawingBuffer is reset.  My first attempt was to try to update the compositor via a call to setNeedsStyleRecalc() in HTMLCanvasElement.cpp:

251c251,256
<             if (hadImageBuffer)
---
>             if (hadImageBuffer) {
> #if USE(IOSURFACE_CANVAS_BACKING_STORE) || (ENABLE(ACCELERATED_2D_CANVAS) && USE(ACCELERATED_COMPOSITING))
>                 // Force a compositor update as well.
>                 if (m_context)
>                     setNeedsStyleRecalc(SyntheticStyleChange);
> #endif
252a258,259
>             }
> 

But this seems to cause the compositor to redraw the entire page.  So the canvas rectangle is white (good), but the rest of the page is full intensity, i.e., not covered by the half-alpha mask which indicates non-repainted areas (bad).
Comment 2 Stephen White 2011-02-16 11:53:50 PST
Created attachment 82670 [details]
Patch
Comment 3 James Robinson 2011-02-16 11:56:33 PST
Comment on attachment 82670 [details]
Patch

Odd - the repaint() alone should be enough to force a recomposite.  What happens on normal draw calls?
Comment 4 Stephen White 2011-02-16 12:00:08 PST
Comment on attachment 82670 [details]
Patch

Taking r? off this, since I'm not sure it's the correct solution yet.  Just wanted to put it in context.
Comment 5 Stephen White 2011-02-16 15:22:50 PST
Created attachment 82707 [details]
Patch
Comment 6 Stephen White 2011-02-16 15:26:10 PST
(In reply to comment #5)
> Created an attachment (id=82707) [details]
> Patch

I'm more confident in this fix.  This basically does the same thing that CanvasRenderingContext2D::didDraw() does, by calling contentChanged() on the RenderLayer.  This still leaves the pixel results wrong, since the compositor doesn't seem to be compatible with repaint tests (it redraws the whole image), but at least the canvas reset is correctly showing up now (white box instead of green).
Comment 7 James Robinson 2011-02-17 11:09:58 PST
Comment on attachment 82707 [details]
Patch

R=me.

Thought: could we call didDraw(FloatRect(canvas()->width(), canvas->height()) ?
Comment 8 Stephen White 2011-02-17 11:19:58 PST
(In reply to comment #7)
> (From update of attachment 82707 [details])
> R=me.
> 
> Thought: could we call didDraw(FloatRect(canvas()->width(), canvas->height()) ?

We probably could, but this might also affect the software path, which I don't believe needs this change.
Comment 9 Stephen White 2011-02-17 15:57:08 PST
The commit queue kept failing on unrelated tests, so I landed this manually as http://trac.webkit.org/changeset/78922.
Comment 10 Eric Seidel (no email) 2011-02-17 16:55:45 PST
The cq was waiting for the tree to be green.  It runs "hot" in that it keeps trying until success.  It didn't ever "fail" this patch or it would have noted so in the bug.
Comment 11 Naoki Takano 2011-02-25 16:55:40 PST
Still the tree info fails but the bug itself looks correct.

So we have to 

(In reply to comment #10)
> The cq was waiting for the tree to be green.  It runs "hot" in that it keeps trying until success.  It didn't ever "fail" this patch or it would have noted so in the bug.
Comment 12 Naoki Takano 2011-02-25 16:58:14 PST
Oops, sorry, I miss-operated.

Still the tree info fails but the bug itself looks gone.
 
So we have to correct "expected" tree files.

I guess, not only setWidthResetAfterForcedRender.html, but also other layout test fail because the tree result is unexpected.

(In reply to comment #11)
> 
> (In reply to comment #10)
> > The cq was waiting for the tree to be green.  It runs "hot" in that it keeps trying until success.  It didn't ever "fail" this patch or it would have noted so in the bug.
Comment 13 Stephen White 2011-02-28 08:44:54 PST
(In reply to comment #12)
> Oops, sorry, I miss-operated.
> 
> Still the tree info fails but the bug itself looks gone.
> 
> So we have to correct "expected" tree files.
> 
> I guess, not only setWidthResetAfterForcedRender.html, but also other layout test fail because the tree result is unexpected.

Although the results are correct in Chrome (same result in software or hardware), the test is still failing since LayoutTestController does not seem to do the right thing for composited layers.  I think we need a way to make LayoutTestController.display() (or more precisely, WebViewHost::displayRepaintMask) write its results into the composited layer, rather than on top of the framebuffer.

So in this case, we shouldn't rebaseline (commit a new expected result), since the result is still wrong until LayoutTestController is fixed.

> (In reply to comment #11)
> > 
> > (In reply to comment #10)
> > > The cq was waiting for the tree to be green.  It runs "hot" in that it keeps trying until success.  It didn't ever "fail" this patch or it would have noted so in the bug.
Comment 14 Kenneth Russell 2011-02-28 12:45:19 PST
(In reply to comment #13)
> Although the results are correct in Chrome (same result in software or hardware), the test is still failing since LayoutTestController does not seem to do the right thing for composited layers.  I think we need a way to make LayoutTestController.display() (or more precisely, WebViewHost::displayRepaintMask) write its results into the composited layer, rather than on top of the framebuffer.
> 
> So in this case, we shouldn't rebaseline (commit a new expected result), since the result is still wrong until LayoutTestController is fixed.

Is there a bug filed about this issue with the LayoutTestController and the compositor? If not, could you file one?