Bug 70379 - Canvas filling paths or rects need to be invalidate larger rects for some compositing modes.
Summary: Canvas filling paths or rects need to be invalidate larger rects for some com...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Wells
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-18 16:14 PDT by Ben Wells
Modified: 2011-11-02 12:04 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.69 KB, patch)
2011-10-31 20:35 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch with test (9.57 KB, patch)
2011-10-31 23:01 PDT, Ben Wells
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Wells 2011-10-18 16:14:01 PDT
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.
Comment 1 Ben Wells 2011-10-31 20:35:29 PDT
Created attachment 113124 [details]
Patch
Comment 2 James Robinson 2011-10-31 21:02:01 PDT
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.
Comment 3 Ben Wells 2011-10-31 23:01:58 PDT
Created attachment 113129 [details]
Patch with test
Comment 4 Stephen White 2011-11-01 06:53:29 PDT
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.
Comment 5 Ben Wells 2011-11-01 17:32:33 PDT
(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.
Comment 6 James Robinson 2011-11-01 23:34:30 PDT
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.
Comment 7 James Robinson 2011-11-01 23:35:55 PDT
Comment on attachment 113129 [details]
Patch with test

R=me
Comment 8 noel gordon 2011-11-01 23:46:20 PDT
Comment on attachment 113129 [details]
Patch with test

cq+ following jamesr's r+
Comment 9 WebKit Review Bot 2011-11-02 00:54:35 PDT
Comment on attachment 113129 [details]
Patch with test

Clearing flags on attachment: 113129

Committed r99043: <http://trac.webkit.org/changeset/99043>
Comment 10 WebKit Review Bot 2011-11-02 00:54:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Stephen White 2011-11-02 06:54:11 PDT
(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.
Comment 12 James Robinson 2011-11-02 12:04:50 PDT
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.