Make RTree handle the case where the playback canvas has empty clip bounds

Make RTree handle the case where the playback canvas has empty clip bounds

BUG=skia:3074
Committed: https://skia.googlesource.com/skia/+/a41d3c33bc4e335dd4702bb9915fdf31d31d6c24

[Justin Novosad, 2014-10-28 17:26:56]

junov@chromium.org changed reviewers:
+ mtklein@google.com

[Justin Novosad, 2014-10-28 17:26:56]

PTAL

[mtklein, 2014-10-28 17:29:44]

lgtm

Out of curiosity, what's getting us to an empty clip in the first place?

https://codereview.chromium.org/678303004/diff/20001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/678303004/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:24: if (!canvas->getClipBounds(&query))
{} ?

[Justin Novosad, 2014-10-28 17:39:06]

On 2014/10/28 17:29:44, mtklein wrote:
> lgtm
> 
> Out of curiosity, what's getting us to an empty clip in the first place?
> 
> https://codereview.chromium.org/678303004/diff/20001/src/core/SkRecordDraw.cpp
> File src/core/SkRecordDraw.cpp (right):
> 
>
https://codereview.chromium.org/678303004/diff/20001/src/core/SkRecordDraw.cp...
> src/core/SkRecordDraw.cpp:24: if (!canvas->getClipBounds(&query))
> {} ?

It's the Chrome compositor + display lis canvas in Blink that is creating the
problem. Basically, we have an SkPicture with an RTree that is recorded (via
drawPicture) into an SkPicture that uses SkTileGrid.  The SkTileGrid may produce
false positives when the query rectangle is not perfectly aligned with the grid.
In the the case I am interested in, the false-positive is a drawPicture call
that is inside a saveLayer. the saveLayer does not intersect with the compositor
tile, so we get empty clip bounds.

[reed1, 2014-10-28 17:39:51]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-10-28 17:39:52]

https://codereview.chromium.org/678303004/diff/20001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/678303004/diff/20001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1550: SkCanvas* canvas = recorder.beginRecording(3, 3);
we seem to make this picture three times, but only wi/ diff opts to the "begin"
method. Can we refactor this to not have so much copy/paste? It might also make
it easy to later extend for other bbox options (or diff picture content).

[Justin Novosad, 2014-10-28 17:40:57]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2014-10-28 17:41:00]

The CQ bit was unchecked by junov@chromium.org

[mtklein, 2014-10-28 17:47:27]

> It's the Chrome compositor + display lis canvas in Blink that is creating the
> problem. Basically, we have an SkPicture with an RTree that is recorded (via
> drawPicture) into an SkPicture that uses SkTileGrid.  The SkTileGrid may
produce
> false positives when the query rectangle is not perfectly aligned with the
grid.
> In the the case I am interested in, the false-positive is a drawPicture call
> that is inside a saveLayer. the saveLayer does not intersect with the
compositor
> tile, so we get empty clip bounds.

I see.  I wasn't aware Chrome was using SkRTree.

I've been toying around with replacing SkTileGrid with SkRTree or perhaps an
even simpler brute force approach.  
It's early still, but it seems we can cut a big chunk of time out of recording
while keeping roughly the same playback speed.
All else equal, I'm inclined to go with a brute force approach if only because
it's harder to write bugs in fewer lines of code.
If brute forcing this really does end up performing just as well, it'll let us
remove the entire concept of an SkBBH.

[Justin Novosad, 2014-10-28 18:55:36]

On 2014/10/28 17:47:27, mtklein wrote:
> > It's the Chrome compositor + display lis canvas in Blink that is creating
the
> > problem. Basically, we have an SkPicture with an RTree that is recorded (via
> > drawPicture) into an SkPicture that uses SkTileGrid.  The SkTileGrid may
> produce
> > false positives when the query rectangle is not perfectly aligned with the
> grid.
> > In the the case I am interested in, the false-positive is a drawPicture call
> > that is inside a saveLayer. the saveLayer does not intersect with the
> compositor
> > tile, so we get empty clip bounds.
> 
> I see.  I wasn't aware Chrome was using SkRTree.
> 
> I've been toying around with replacing SkTileGrid with SkRTree or perhaps an
> even simpler brute force approach.  
> It's early still, but it seems we can cut a big chunk of time out of recording
> while keeping roughly the same playback speed.
> All else equal, I'm inclined to go with a brute force approach if only because
> it's harder to write bugs in fewer lines of code.
> If brute forcing this really does end up performing just as well, it'll let us
> remove the entire concept of an SkBBH.

Yeah, The BBHs were designed to cater to the compositor-skia interactions we had
two years ago.  A lot has changed since then. I think they can be made obsolote
by making blink and the compositor smarter about how content is organised.  I
guess we'll see...

[Justin Novosad, 2014-10-28 20:16:04]

New Patch.

Refactored the test in a way that has no copied boilerplate code.

[Justin Novosad, 2014-10-29 15:08:48]

ping.  Like the test now?

[reed1, 2014-10-29 15:56:15]

https://codereview.chromium.org/678303004/diff/60001/tests/PictureBBHTest.cpp
File tests/PictureBBHTest.cpp (right):

https://codereview.chromium.org/678303004/diff/60001/tests/PictureBBHTest.cpp...
tests/PictureBBHTest.cpp:27: virtual void resetCanvas(SkCanvas& resultCanvas) =
0;
I assumed we could always assert that nothing drew, so it seems we could
eliminate the overhead of viruals for resetCanvas and validateResult, since we
just wanted to specialize on what was drawn. In that model, run() would erase to
GREEN and it could assert GREEN afterwards.

https://codereview.chromium.org/678303004/diff/60001/tests/PictureBBHTest.cpp...
tests/PictureBBHTest.cpp:33: virtual ~PictureBBHTestBase() { }
nit: skia puts its destructor right after the constructor(s)

https://codereview.chromium.org/678303004/diff/60001/tests/PictureBBHTest.cpp...
tests/PictureBBHTest.cpp:52: resetCanvas(playbackCanvas);
nit: this->resetCanvas(...) for methods

https://codereview.chromium.org/678303004/diff/60001/tests/PictureBBHTest.cpp...
tests/PictureBBHTest.cpp:73: virtual void resetCanvas(SkCanvas& resultCanvas) {
}
nit: SK_OVERRIDE in the subclasses

https://codereview.chromium.org/678303004/diff/60001/tests/PictureBBHTest.cpp...
tests/PictureBBHTest.cpp:90: resultCanvas.clipRect(SkRect::MakeXYWH(10, 10, 1,
1), SkRegion::kIntersect_Op);
seems like we can fold this clip call into the recordCommands(), eliminating the
need for this virtual entirely.

[Justin Novosad, 2014-10-29 19:39:05]

Done.

[Justin Novosad, 2014-10-30 16:05:21]

Review ping.  This one is time sensitive for M-40. thx.

[reed1, 2014-10-30 16:35:23]

wow, much simpler/easier-to-grok, thanks.

lgtm

[Justin Novosad, 2014-10-30 16:53:16]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-10-30 16:53:33]

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

[commit-bot: I haz the power, 2014-10-30 16:56:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 16:56:52]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[Justin Novosad, 2014-10-30 17:27:45]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:27:57]

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

[commit-bot: I haz the power, 2014-10-30 17:38:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:38:06]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[Justin Novosad, 2014-10-30 18:07:56]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-10-30 18:08:11]

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

[commit-bot: I haz the power, 2014-10-30 18:44:24]

Committed patchset #7 (id:120001) as a41d3c33bc4e335dd4702bb9915fdf31d31d6c24