Avoid false-positives of under-invalidation checking

Avoid false-positives of under-invalidation checking

HTMLCanvasPainter may produce pictures having the same contents but
different representations. Add CheckBitmap mode of under-invalidation
checking for the case. If cull rects are the same, check bitmaps to
avoid false-positives.

BUG=486845
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196259

[Xianzhu, 2015-06-01 18:29:25]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, junov@chromium.org

[chrishtr, 2015-06-01 19:04:28]

https://codereview.chromium.org/1160223004/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1160223004/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemList.cpp:354: SkRect rect =
newPicture->cullRect();
I think this would induce too many false negatives.

How about just putting a bit on a drawing recorder in ENABLE(ASSERT) mode that
can be set by canvas when it's doing a representational change?

[Xianzhu, 2015-06-01 19:10:15]

https://codereview.chromium.org/1160223004/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1160223004/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemList.cpp:354: SkRect rect =
newPicture->cullRect();
On 2015/06/01 19:04:28, chrishtr wrote:
> I think this would induce too many false negatives.
>

Could you give more details about the case of false negatives?

> How about just putting a bit on a drawing recorder in ENABLE(ASSERT) mode that
> can be set by canvas when it's doing a representational change?

[Xianzhu, 2015-06-01 19:12:32]

Anyway, just noticed that the method won't work when we remove cullRect. Trying
your method.

[Xianzhu, 2015-06-01 19:15:02]

On 2015/06/01 19:12:32, Xianzhu wrote:
> Anyway, just noticed that the method won't work when we remove cullRect.
Trying
> your method.

Also remembered that I have already added the skipUnderInvalidationChecking flag
in DrawingRecorder and DrawingDisplayItem :)

[chrishtr, 2015-06-01 19:17:47]

On 2015/06/01 at 19:15:02, wangxianzhu wrote:
> On 2015/06/01 19:12:32, Xianzhu wrote:
> > Anyway, just noticed that the method won't work when we remove cullRect.
Trying
> > your method.
> 
> Also remembered that I have already added the skipUnderInvalidationChecking
flag in DrawingRecorder and DrawingDisplayItem :)

Cool.

The use case I was concerned about was re-drawing content that happens to look
the same but only by accident.

[Xianzhu, 2015-06-01 21:12:42]

Now the CL compares bitmaps of pictures for HTML canvas painting only.

Ptal.

[chrishtr, 2015-06-01 21:35:37]

https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/BoxPa...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/BoxPa...
Source/core/paint/BoxPainter.cpp:104:
recorder.setUnderInvalidationCheckingMode(DrawingDisplayItem::DontCheck);
Wrong indentation.

https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/HTMLC...
File Source/core/paint/HTMLCanvasPainter.cpp (right):

https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/HTMLC...
Source/core/paint/HTMLCanvasPainter.cpp:32:
drawingRecorder.setUnderInvalidationCheckingMode(DrawingDisplayItem::CheckBitmap);
Indentation?

[Xianzhu, 2015-06-01 21:43:18]

https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/BoxPa...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/BoxPa...
Source/core/paint/BoxPainter.cpp:104:
recorder.setUnderInvalidationCheckingMode(DrawingDisplayItem::DontCheck);
On 2015/06/01 21:35:37, chrishtr wrote:
> Wrong indentation.

It looks correct to me (4-space indent at line 103, 8-space indent at line 104).

https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/HTMLC...
File Source/core/paint/HTMLCanvasPainter.cpp (right):

https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/HTMLC...
Source/core/paint/HTMLCanvasPainter.cpp:32:
drawingRecorder.setUnderInvalidationCheckingMode(DrawingDisplayItem::CheckBitmap);
On 2015/06/01 21:35:37, chrishtr wrote:
> Indentation?

Isn't 4-space indent correct?

[chrishtr, 2015-06-01 21:46:28]

On 2015/06/01 at 21:43:18, wangxianzhu wrote:
>
https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/BoxPa...
> File Source/core/paint/BoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/BoxPa...
> Source/core/paint/BoxPainter.cpp:104:
recorder.setUnderInvalidationCheckingMode(DrawingDisplayItem::DontCheck);
> On 2015/06/01 21:35:37, chrishtr wrote:
> > Wrong indentation.
> 
> It looks correct to me (4-space indent at line 103, 8-space indent at line
104).
> 
>
https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/HTMLC...
> File Source/core/paint/HTMLCanvasPainter.cpp (right):
> 
>
https://codereview.chromium.org/1160223004/diff/40001/Source/core/paint/HTMLC...
> Source/core/paint/HTMLCanvasPainter.cpp:32:
drawingRecorder.setUnderInvalidationCheckingMode(DrawingDisplayItem::CheckBitmap);
> On 2015/06/01 21:35:37, chrishtr wrote:
> > Indentation?
> 
> Isn't 4-space indent correct?

It was an artifact of the code review tool's view, sorry. Comments retracted.

[chrishtr, 2015-06-01 21:46:37]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-01 21:46:38]

lgtm

[Justin Novosad, 2015-06-01 21:47:34]

https://codereview.chromium.org/1160223004/diff/40001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1160223004/diff/40001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:358: case
DrawingDisplayItem::CheckBitmap:
For now, this strategy will work. In the near future, this method may end up
comparing ganesh vs non-ganesh renders, which may have subtle differences due to
anti-aliasing precision.  I think it will be possible to solve by adding a new
mode here.

[Justin Novosad, 2015-06-01 21:47:47]

lgtm

[Xianzhu, 2015-06-01 22:23:19]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2015-06-01 22:23:23]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-06-01 23:08:36]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2015-06-01 23:08:44]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[Xianzhu, 2015-06-01 23:12:35]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-06-01 23:12:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1160223004/#ps80001
(title: "Fix build break (release w/ assert)")

[commit-bot: I haz the power, 2015-06-01 23:34:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1160223004/80001

[commit-bot: I haz the power, 2015-06-02 00:23:03]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196259