Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
971803002:
Update SkPicture cull rects with RTree information (Closed)
|
Created:
5 years, 9 months ago by Stephen Chennney Modified:
5 years, 9 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionUpdate SkPicture cull rects with RTree information
When computed, the RTree for an SkPicture will have a root
bounds that reflects the best bounding information available,
rather than the best estimate at the time the picture recorder
is created. Given that creators frequently don't know ahead of
time what will be drawn, the RTree bound is often tighter.
Perf testing on Chrome indicates a small raster performance
advantage. For upcoming painting changes in Chrome the
performance advantage is much larger.
BUG=
Committed: https://skia.googlesource.com/skia/+/2dd3b6647dc726f36fd8774b3d0d2e83b493aeac
Committed: https://skia.googlesource.com/skia/+/23d8593f8127411d9d687b4565b34b4ecd6b11d3
Patch Set 1 : Removed unchanged file. #Patch Set 2 : Fix test BBHs #Patch Set 3 : More trybot debugging. #Patch Set 4 : Still trying. #Patch Set 5 : Fix the assertion #Patch Set 6 : Fix bound for drawSprite #Patch Set 7 : Fix the bug breaking CC unit tests. #
Total comments: 1
Patch Set 8 : Now with all changes, and better clip logic #
Total comments: 1
Patch Set 9 : Fix extra line #
Messages
Total messages: 51 (14 generated)
Patchset #1 (id:1) has been deleted
schenney@chromium.org changed reviewers: + mtklein@skia.org
I tried this to good effect. Landing it will make RTree construction from display lists much more efficient.
schenney@chromium.org changed reviewers: + mtklein@google.com - mtklein@skia.org
Not sure if you noticed this.
On 2015/03/03 17:55:55, Stephen Chenney wrote: > Not sure if you noticed this. Oh, yeah, all skia.org email has been broken for a couple weeks. Looking now.
The CQ bit was checked by mtklein@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/971803002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
Gotta fix the tests. Sorry.
On 2015/03/03 18:04:55, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia > (JOB_FAILED, > http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...) Ah, right, we have a couple BBHs in tests.
The CQ bit was checked by schenney@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/971803002/#ps40001 (title: "Fix test BBHs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/971803002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
Patchset #3 (id:60001) has been deleted
Mike, I think this needs re-review by now. Should I add a test? The only issue is that with skia-in-chrome checkout I can't seem to build the test software. I'll have to do a new checkout.
On 2015/03/03 21:34:26, Stephen Chenney wrote: > Mike, I think this needs re-review by now. Should I add a test? The only issue > is that with skia-in-chrome checkout I can't seem to build the test software. > I'll have to do a new checkout. Spoke too soon. Looks like that test needs more thinking.
On 2015/03/03 21:35:41, Stephen Chenney wrote: > On 2015/03/03 21:34:26, Stephen Chenney wrote: > > Mike, I think this needs re-review by now. Should I add a test? The only issue > > is that with skia-in-chrome checkout I can't seem to build the test software. > > I'll have to do a new checkout. > > Spoke too soon. Looks like that test needs more thinking. OK. Will wait. The most interesting thing to me is that the assertion is failing.
On 2015/03/03 21:46:03, mtklein wrote: > On 2015/03/03 21:35:41, Stephen Chenney wrote: > > On 2015/03/03 21:34:26, Stephen Chenney wrote: > > > Mike, I think this needs re-review by now. Should I add a test? The only > issue > > > is that with skia-in-chrome checkout I can't seem to build the test > software. > > > I'll have to do a new checkout. > > > > Spoke too soon. Looks like that test needs more thinking. > > OK. Will wait. The most interesting thing to me is that the assertion is > failing. I'm trying to figure out if it's just some odd rounding thing. Should not assert as far as I can tell.
On 2015/03/03 21:47:17, Stephen Chenney wrote: > On 2015/03/03 21:46:03, mtklein wrote: > > On 2015/03/03 21:35:41, Stephen Chenney wrote: > > > On 2015/03/03 21:34:26, Stephen Chenney wrote: > > > > Mike, I think this needs re-review by now. Should I add a test? The only > > issue > > > > is that with skia-in-chrome checkout I can't seem to build the test > > software. > > > > I'll have to do a new checkout. > > > > > > Spoke too soon. Looks like that test needs more thinking. > > > > OK. Will wait. The most interesting thing to me is that the assertion is > > failing. > > I'm trying to figure out if it's just some odd rounding thing. Should not assert > as far as I can tell. Right. Logically I totally agree. IIRC, we start with the cull and then only possibly intersect it down. Are we just being stymied by one of the two rects being empty?
On 2015/03/03 21:50:28, mtklein wrote: > On 2015/03/03 21:47:17, Stephen Chenney wrote: > > On 2015/03/03 21:46:03, mtklein wrote: > > > On 2015/03/03 21:35:41, Stephen Chenney wrote: > > > > On 2015/03/03 21:34:26, Stephen Chenney wrote: > > > > > Mike, I think this needs re-review by now. Should I add a test? The only > > > issue > > > > > is that with skia-in-chrome checkout I can't seem to build the test > > > software. > > > > > I'll have to do a new checkout. > > > > > > > > Spoke too soon. Looks like that test needs more thinking. > > > > > > OK. Will wait. The most interesting thing to me is that the assertion is > > > failing. > > > > I'm trying to figure out if it's just some odd rounding thing. Should not > assert > > as far as I can tell. > > Right. Logically I totally agree. IIRC, we start with the cull and then only > possibly intersect it down. Are we just being stymied by one of the two rects > being empty? I've got myself set up to build the tests now (turns out you can put a .gclient in the third_party/skia directory of chrome). I'll try sort out the problem in the morning.
> I've got myself set up to build the tests now (turns out you can put a .gclient > in the third_party/skia directory of chrome). I'll try sort out the problem in > the morning. Another thing you can do is from chromium/src/third_party/skia run tools/git-sync-deps. It works around the weird DEPS in DEPS in gclient in gclient issues by not using gclient to sync the deps. Then run ./gyp_skia as normal.
At last. Turns out the SkRect::contains fails if either rect is empty, when I would expect that every non-empty rect contains an empty rect within it. Another l.g.t.m is in order, I think.
On 2015/03/04 16:05:40, Stephen Chenney wrote: > At last. Turns out the SkRect::contains fails if either rect is empty, when I > would expect that every non-empty rect contains an empty rect within it. > > Another l.g.t.m is in order, I think. How do I get the skps etc needed to recreate the builder behavior?
On 2015/03/04 16:26:51, Stephen Chenney wrote: > On 2015/03/04 16:05:40, Stephen Chenney wrote: > > At last. Turns out the SkRect::contains fails if either rect is empty, when I > > would expect that every non-empty rect contains an empty rect within it. > > > > Another l.g.t.m is in order, I think. > > How do I get the skps etc needed to recreate the builder behavior? I run bin/fetch-skps, again from skia/.
On 2015/03/04 16:34:46, mtklein wrote: > On 2015/03/04 16:26:51, Stephen Chenney wrote: > > On 2015/03/04 16:05:40, Stephen Chenney wrote: > > > At last. Turns out the SkRect::contains fails if either rect is empty, when > I > > > would expect that every non-empty rect contains an empty rect within it. > > > > > > Another l.g.t.m is in order, I think. > > > > How do I get the skps etc needed to recreate the builder behavior? > > I run bin/fetch-skps, again from skia/.
On 2015/03/04 16:35:04, mtklein wrote: > On 2015/03/04 16:34:46, mtklein wrote: > > On 2015/03/04 16:26:51, Stephen Chenney wrote: > > > On 2015/03/04 16:05:40, Stephen Chenney wrote: > > > > At last. Turns out the SkRect::contains fails if either rect is empty, > when > > I > > > > would expect that every non-empty rect contains an empty rect within it. > > > > > > > > Another l.g.t.m is in order, I think. > > > > > > How do I get the skps etc needed to recreate the builder behavior? > > > > I run bin/fetch-skps, again from skia/. You may need to authenticate yourself to Google Storage with your @google.com credentials for that to work.
Looks like this is ready but, Mike, I'd like you to approve again given I made some changes to tests etc.
lgtm
The CQ bit was checked by schenney@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/971803002/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://skia.googlesource.com/skia/+/2dd3b6647dc726f36fd8774b3d0d2e83b493aeac
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/977413003/ by joshualitt@google.com. The reason for reverting is: Might be breaking deps roll.
Message was sent while issue was closed.
On 2015/03/05 20:17:52, joshualitt wrote: > A revert of this CL (patchset #6 id:140001) has been created in > https://codereview.chromium.org/977413003/ by mailto:joshualitt@google.com. > > The reason for reverting is: Might be breaking deps roll. Yeah, that was definitely this CL. The new assert we added is failing in cc_unittests.
Message was sent while issue was closed.
On 2015/03/05 20:35:04, mtklein wrote: > On 2015/03/05 20:17:52, joshualitt wrote: > > A revert of this CL (patchset #6 id:140001) has been created in > > https://codereview.chromium.org/977413003/ by mailto:joshualitt@google.com. > > > > The reason for reverting is: Might be breaking deps roll. > > Yeah, that was definitely this CL. The new assert we added is failing in > cc_unittests. Yep. Turns out the cc unit tests use a clip bigger than the cull rect, which we weren't checking for. I've updated to address the problem, now cc_unittests pass as do dm tests. Let me know if you want me to pull that clip.intersect(fCullRect) code block into a method.
Seems like a good idea. Looks like the rest of the CL has gone missing? https://codereview.chromium.org/971803002/diff/160001/src/core/SkRecordDraw.cpp File src/core/SkRecordDraw.cpp (right): https://codereview.chromium.org/971803002/diff/160001/src/core/SkRecordDraw.c... src/core/SkRecordDraw.cpp:252: if (isBoundedClip) { Let's definitely create a little method for the assert on 261+262. SkASSERT(this->currentClipInsideCullRect()) or something like that. I'm okay repeating this logic here if we tighten it up a bit, and I also am ok with extracting it as its own little method. Here's what I'd suggest if we leave this in place: if (this->adjustForSaveLayerPaints(&clip, ...)) { fCurrentClipBounds = clip.intersect(fCullRect) ? clip : Bounds::MakeEmpty(); } else { fCurrentClipBounds = fCullRect; }
On 2015/03/06 18:18:50, mtklein wrote: > Seems like a good idea. > > Looks like the rest of the CL has gone missing? Yeah, must have been a bad merge. > https://codereview.chromium.org/971803002/diff/160001/src/core/SkRecordDraw.cpp > File src/core/SkRecordDraw.cpp (right): > > https://codereview.chromium.org/971803002/diff/160001/src/core/SkRecordDraw.c... > src/core/SkRecordDraw.cpp:252: if (isBoundedClip) { > Let's definitely create a little method for the assert on 261+262. > SkASSERT(this->currentClipInsideCullRect()) or something like that. I didn't intend to leave the asserts in. We'll catch them in the picture recorder assert. > I'm okay repeating this logic here if we tighten it up a bit, and I also am ok > with extracting it as its own little method. Here's what I'd suggest if we > leave this in place: > > if (this->adjustForSaveLayerPaints(&clip, ...)) { > fCurrentClipBounds = clip.intersect(fCullRect) ? clip : Bounds::MakeEmpty(); > } else { > fCurrentClipBounds = fCullRect; > } I've tightened it. not sure why I was having so much trouble yesterday.
lgtm https://codereview.chromium.org/971803002/diff/180001/src/core/SkRecordDraw.cpp File src/core/SkRecordDraw.cpp (right): https://codereview.chromium.org/971803002/diff/180001/src/core/SkRecordDraw.c... src/core/SkRecordDraw.cpp:286: fBounds[fCurrentOp] = this->popSaveBlock(); Might want to put this guy back on one line?
The CQ bit was checked by schenney@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/971803002/#ps200001 (title: "Fix extra line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/971803002/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://skia.googlesource.com/skia/+/23d8593f8127411d9d687b4565b34b4ecd6b11d3
Message was sent while issue was closed.
reed@chromium.org changed reviewers: + reed@chromium.org
Message was sent while issue was closed.
Are we able to produce a bench that shows the performance gain/loss, so we can track this?
Message was sent while issue was closed.
On 2015/03/07 14:07:04, reed2 wrote: > Are we able to produce a bench that shows the performance gain/loss, so we can > track this? We could make something that would obviously detect that the optimization was removed. Is that your concern? We can also get an almost arbitrary performance improvement in a carefully constructed example. To me that's less valuable because I would expect performance to be constant under almost all changes (except removing the optimization). There are cases where an improvement in the bounds for specific draw opcodes would generate some kind of incremental improvement, but it's impossible to know ahead of time how to construct a perf test that would show that, and continue to show it over time. I guess I should also see if this change affected any of the existing Skia perf tests. Chrome perf takes a while to filter through.
Message was sent while issue was closed.
reed@google.com changed reviewers: + reed@google.com
Message was sent while issue was closed.
If this is an optimization, then I'm assuming we can measure something that gets faster. I was just asking if we had such a measurement in-place in Skia, so we can see it, and know if subsequent changes improve or reduce that. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
