Bug 70379 – Canvas filling paths or rects need to be invalidate larger rects for some compositing modes.

• Component: Canvas
• Status: RESOLVED FIXED

[Ben Wells]

After drawing the canvas code uses calls to didDraw to invalidate what has changed. For composite modes which affect the whole canvas (e.g. Source-In), the whole canvas needs to be invalidated.

This isn't happening for fill or fillRect, which do implement the composite modes correctly. These operations, and any other drawing operations which do implement these composite modes correctly should call didDraw with the whole canvas.

[Ben Wells]

Created attachment 113124 [details]
Patch

[James Robinson]

Comment on attachment 113124 [details]
Patch

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

Need tests

> Source/WebCore/ChangeLog:8
> +        No new tests.

the way we normally test invalidation rects like this is via repaint tests - paint once, then do a draw, then paint again and see if the repainted area is correct. we probably already have some for canvas draws already.

[Ben Wells]

Created attachment 113129 [details]
Patch with test

[Stephen White]

Comment on attachment 113129 [details]
Patch with test

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

> LayoutTests/ChangeLog:10
> +        * platform/chromium-gpu-linux/fast/canvas/canvas-composite-fill-repaint-expected.png: Added.

Hmm.  This result image looks wrong, but is probably not your fault -- the compositor doesn't support repaint tests, IIRC.

Since apparently we're switching to a policy of checking in bad results anyway, this is ok, but you should probably add a line to test_expectations documenting the problem.

[Ben Wells]

(In reply to comment #4)
> Hmm.  This result image looks wrong, but is probably not your fault -- the compositor doesn't support repaint tests, IIRC.
> 
> Since apparently we're switching to a policy of checking in bad results anyway, this is ok, but you should probably add a line to test_expectations documenting the problem.

The GPU compositor always redraws the whole screen (if I understand James' comment on bug 67760 correctly).

The problem being fixed is that not enough of the canvas is redrawn after some operations. Redrawing too much is correct, but may be inefficient. Is that right? If so the expected GPU result is good.

If its not right (i.e. the GPU result is a failure) ... I wasn't aware of a policy to check in incorrect results. Is this how I would document the problem in test_expectations:
// GPU doesn't support repaint tests; checked in expected result is not correct.
BUG67760 GPU : fast/canvas/canvas-composite-fill-repaint-expected.png = PASS

Seems more complicated than putting in an expected image failure.

[James Robinson]

That sounds correct. I don't see any reason to put a line in test_expectations, that png is what we expect for this test.

[James Robinson]

Comment on attachment 113129 [details]
Patch with test

R=me

[noel gordon]

Comment on attachment 113129 [details]
Patch with test

cq+ following jamesr's r+

[WebKit Review Bot]

Comment on attachment 113129 [details]
Patch with test

Clearing flags on attachment: 113129

Committed r99043: <http://trac.webkit.org/changeset/99043>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

(In reply to comment #6)
> That sounds correct. I don't see any reason to put a line in test_expectations, that png is what we expect for this test.

Hmm.  My thought was that we could get repaint tests to work by having the compositor draw the grey rectangle over the layer of interest, rather than on the compositor's result.  Then, the compositor would upload the whole layer (since it would be invalidated), but it would contain the grey rectangle in the non-redrawn regions.  Would that not work?

If not, running repaint tests on the compositor seems like a waste of time, since they're not telling us what we want them to tell us, namely that the invalidated portion (and only the invalidated portion) is being redrawn.

[James Robinson]

I think the repaint tests still "work" in that we can verify if the final pixel output is what we want. The repaint tests are for two things:

1.) What is the final pixel output?
2.) What portion of the final output was painted on the last frame?

We always paint every pixel every frame in compositing mode so part (2) isn't really tested, but part 1 is still nice.