Bug 54561 – [chromium] Layout Test fast/canvas/setWidthResetAfterForcedRender.html fails on accelerated 2D canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Stephen White]

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

[Stephen White]

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).

[Stephen White]

Created attachment 82670 [details]
Patch

[James Robinson]

Comment on attachment 82670 [details]
Patch

Odd - the repaint() alone should be enough to force a recomposite.  What happens on normal draw calls?

[Stephen White]

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.

[Stephen White]

Created attachment 82707 [details]
Patch

[Stephen White]

(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).

[James Robinson]

Comment on attachment 82707 [details]
Patch

R=me.

Thought: could we call didDraw(FloatRect(canvas()->width(), canvas->height()) ?

[Stephen White]

(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.

[Stephen White]

The commit queue kept failing on unrelated tests, so I landed this manually as http://trac.webkit.org/changeset/78922.

[Eric Seidel (no email)]

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.

[Naoki Takano]

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.

[Naoki Takano]

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.

[Stephen White]

(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.

[Kenneth Russell]

(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?