Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2132073002:
Pre-crop filled rects to avoid scissor (Closed)
|
Created:
4 years, 5 months ago by csmartdalton Modified:
4 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionPre-crop filled rects to avoid scissor
Updates GrDrawContext to crop filled rects to the clip bounds before
creating batches for them. Also adds clipping logic to ignore scissor
when the draw falls completely inside. These two changes combined
reduce API traffic and improve batching.
In the future this can and should be improved by switching to floating
point clip boundaries, thus allowing us to throw out non pixel aligned
rectangle clips as well.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002
Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a
Committed: https://skia.googlesource.com/skia/+/86de59f4a99b5f54be0483c60ff0335be55b2bdf
Committed: https://skia.googlesource.com/skia/+/97f6cd5d0fc84dbed7cd8770b79695df83c69444
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 8
Patch Set 3 : Pre-crop filled rects to avoid scissor #
Total comments: 5
Patch Set 4 : addressed comments #Patch Set 5 : rebaes #Patch Set 6 : nearest filtering for the gm #
Messages
Total messages: 41 (14 generated)
Description was changed from ========== Trim rect prototype BUG=skia: ========== to ========== Trim rect prototype BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 ==========
csmartdalton@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com
What would you think of adding a "fuzzyContains" method to SkIRect?
https://codereview.chromium.org/2132073002/diff/20001/include/gpu/GrDrawConte... File include/gpu/GrDrawContext.h (right): https://codereview.chromium.org/2132073002/diff/20001/include/gpu/GrDrawConte... include/gpu/GrDrawContext.h:330: SkAutoTUnref<GrDrawBatch>* batch, I'm not sure I like this approach. Would it be better to return a batch, and set a bool pointer-parameter that says whether to skip the draw? https://codereview.chromium.org/2132073002/diff/20001/src/gpu/GrClipMaskManag... File src/gpu/GrClipMaskManager.cpp (right): https://codereview.chromium.org/2132073002/diff/20001/src/gpu/GrClipMaskManag... src/gpu/GrClipMaskManager.cpp:373: out->makeScissoredStencil(scissorSpaceIBounds); Any reason we weren't passing devBounds here?
https://codereview.chromium.org/2132073002/diff/20001/include/gpu/GrDrawConte... File include/gpu/GrDrawContext.h (right): https://codereview.chromium.org/2132073002/diff/20001/include/gpu/GrDrawConte... include/gpu/GrDrawContext.h:330: SkAutoTUnref<GrDrawBatch>* batch, On 2016/07/08 21:20:45, csmartdalton wrote: > I'm not sure I like this approach. Would it be better to return a batch, and set > a bool pointer-parameter that says whether to skip the draw? If you update to ToT this call should be replaced with a drawFillRect entry point. You should just be able to skip the draw in there.
I'd be in favor of landing the fuzzy contains stuff separately from the rect trimming. It seems like a valuable change on its own. https://codereview.chromium.org/2132073002/diff/20001/include/core/SkRect.h File include/core/SkRect.h (right): https://codereview.chromium.org/2132073002/diff/20001/include/core/SkRect.h#n... include/core/SkRect.h:263: bool fuzzyContains(const SkRect& r, SkScalar fuzz) const; Instead of adding this to the public API, can we just add it as a helper function in GrClipMaskManager.cpp? https://codereview.chromium.org/2132073002/diff/20001/src/gpu/GrClipMaskManag... File src/gpu/GrClipMaskManager.cpp (right): https://codereview.chromium.org/2132073002/diff/20001/src/gpu/GrClipMaskManag... src/gpu/GrClipMaskManager.cpp:271: if (!SkRect::Make(scissorSpaceIBounds).contains(devBounds)) { Here is another spot (This is where we land when the clip is *only* a integerrectangle). https://codereview.chromium.org/2132073002/diff/20001/src/gpu/GrClipMaskManag... src/gpu/GrClipMaskManager.cpp:373: out->makeScissoredStencil(scissorSpaceIBounds); On 2016/07/08 21:20:45, csmartdalton wrote: > Any reason we weren't passing devBounds here? We shouldn't be passing devBounds. This is supposed to be the bounds of the clip, not the bounds of the draw. It is used by GrDrawContext to compute an intersection of the clip and the draw.
Description was changed from ========== Trim rect prototype BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 ========== to ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 ==========
This is ready to go now. https://codereview.chromium.org/2132073002/diff/20001/include/gpu/GrDrawConte... File include/gpu/GrDrawContext.h (right): https://codereview.chromium.org/2132073002/diff/20001/include/gpu/GrDrawConte... include/gpu/GrDrawContext.h:330: SkAutoTUnref<GrDrawBatch>* batch, On 2016/07/08 22:39:48, robertphillips wrote: > On 2016/07/08 21:20:45, csmartdalton wrote: > > I'm not sure I like this approach. Would it be better to return a batch, and > set > > a bool pointer-parameter that says whether to skip the draw? > > If you update to ToT this call should be replaced with a drawFillRect entry > point. You should just be able to skip the draw in there. Done. Much better! https://codereview.chromium.org/2132073002/diff/20001/src/gpu/GrClipMaskManag... File src/gpu/GrClipMaskManager.cpp (right): https://codereview.chromium.org/2132073002/diff/20001/src/gpu/GrClipMaskManag... src/gpu/GrClipMaskManager.cpp:271: if (!SkRect::Make(scissorSpaceIBounds).contains(devBounds)) { On 2016/07/08 22:49:00, bsalomon wrote: > Here is another spot (This is where we land when the clip is *only* a > integerrectangle). Done. Thanks for pointing that out, hopefully it leads to better batching. https://codereview.chromium.org/2132073002/diff/40001/gm/croppedrects.cpp File gm/croppedrects.cpp (right): https://codereview.chromium.org/2132073002/diff/40001/gm/croppedrects.cpp#new... gm/croppedrects.cpp:93: // TODO: assert the draw target only has one batch? Doable?
https://codereview.chromium.org/2132073002/diff/40001/gm/croppedrects.cpp File gm/croppedrects.cpp (right): https://codereview.chromium.org/2132073002/diff/40001/gm/croppedrects.cpp#new... gm/croppedrects.cpp:93: // TODO: assert the draw target only has one batch? On 2016/07/11 03:17:56, csmartdalton wrote: > Doable? We could add something to draw context to get the batch count. However, I'd be a little worried that we might have a clear batch in there or something like that. Maybe this would make more sense post-MDB when fullscreen clears, copies, and discards aren't batches (and when the batch count would really only reflect one GrDC, whereas now all the DCs share a batch array via their common GrDrawTarget). https://codereview.chromium.org/2132073002/diff/40001/src/gpu/GrDrawContext.cpp File src/gpu/GrDrawContext.cpp (right): https://codereview.chromium.org/2132073002/diff/40001/src/gpu/GrDrawContext.c... src/gpu/GrDrawContext.cpp:560: SkRect croppedLocalRect = localRect; What do you think about making this an optional param to crop_fill_rect and doing the clip there?
https://codereview.chromium.org/2132073002/diff/40001/gm/croppedrects.cpp File gm/croppedrects.cpp (right): https://codereview.chromium.org/2132073002/diff/40001/gm/croppedrects.cpp#new... gm/croppedrects.cpp:93: // TODO: assert the draw target only has one batch? On 2016/07/11 13:52:23, bsalomon wrote: > On 2016/07/11 03:17:56, csmartdalton wrote: > > Doable? > > We could add something to draw context to get the batch count. However, I'd be a > little worried that we might have a clear batch in there or something like that. > Maybe this would make more sense post-MDB when fullscreen clears, copies, and > discards aren't batches (and when the batch count would really only reflect one > GrDC, whereas now all the DCs share a batch array via their common > GrDrawTarget). Done. https://codereview.chromium.org/2132073002/diff/40001/src/gpu/GrDrawContext.cpp File src/gpu/GrDrawContext.cpp (right): https://codereview.chromium.org/2132073002/diff/40001/src/gpu/GrDrawContext.c... src/gpu/GrDrawContext.cpp:560: SkRect croppedLocalRect = localRect; On 2016/07/11 13:52:23, bsalomon wrote: > What do you think about making this an optional param to crop_fill_rect and > doing the clip there? Done.
lgtm
The CQ bit was checked by csmartdalton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2132073002/#ps100001 (title: "nearest filtering for the gm")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
The CQ bit was checked by csmartdalton@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 ========== to ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2145573002/ by msarett@google.com. The reason for reverting is: I believe that this is breaking the roll. https://codereview.chromium.org/2141923002 https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel....
Message was sent while issue was closed.
On 2016/07/12 12:40:32, msarett wrote: > A revert of this CL (patchset #6 id:100001) has been created in > https://codereview.chromium.org/2145573002/ by mailto:msarett@google.com. > > The reason for reverting is: I believe that this is breaking the roll. > > https://codereview.chromium.org/2141923002 > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel.... Looks like we fail a cc_unittest. Seeing if I can reproduce it locally on linux.
Message was sent while issue was closed.
On 2016/07/12 14:02:10, bsalomon wrote: > On 2016/07/12 12:40:32, msarett wrote: > > A revert of this CL (patchset #6 id:100001) has been created in > > https://codereview.chromium.org/2145573002/ by mailto:msarett@google.com. > > > > The reason for reverting is: I believe that this is breaking the roll. > > > > https://codereview.chromium.org/2141923002 > > > > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel.... > > Looks like we fail a cc_unittest. Seeing if I can reproduce it locally on linux. This doesn't have to do with the approximate containment but rather the trimming in GrDrawContext. Here is the expected image: https://jsfiddle.net/00v08chz/ (this is actually with the fuzzy scissor test turned off) and here is the actual image: http://jsfiddle.net/yp73nvsm/1/ I can't really see a difference myself
Message was sent while issue was closed.
On 2016/07/12 15:36:03, bsalomon wrote: > On 2016/07/12 14:02:10, bsalomon wrote: > > On 2016/07/12 12:40:32, msarett wrote: > > > A revert of this CL (patchset #6 id:100001) has been created in > > > https://codereview.chromium.org/2145573002/ by mailto:msarett@google.com. > > > > > > The reason for reverting is: I believe that this is breaking the roll. > > > > > > https://codereview.chromium.org/2141923002 > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel.... > > > > Looks like we fail a cc_unittest. Seeing if I can reproduce it locally on > linux. > > This doesn't have to do with the approximate containment but rather the trimming > in GrDrawContext. Here is the expected image: > https://jsfiddle.net/00v08chz/ (this is actually with the fuzzy scissor test > turned off) > > and here is the actual image: > > http://jsfiddle.net/yp73nvsm/1/ > > I can't really see a difference myself And here is the image with the fuzzy scissor test. I still can't see a difference: https://jsfiddle.net/00v08chz/2/
Message was sent while issue was closed.
On 2016/07/12 15:41:53, bsalomon wrote: > On 2016/07/12 15:36:03, bsalomon wrote: > > On 2016/07/12 14:02:10, bsalomon wrote: > > > On 2016/07/12 12:40:32, msarett wrote: > > > > A revert of this CL (patchset #6 id:100001) has been created in > > > > https://codereview.chromium.org/2145573002/ by mailto:msarett@google.com. > > > > > > > > The reason for reverting is: I believe that this is breaking the roll. > > > > > > > > https://codereview.chromium.org/2141923002 > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel.... > > > > > > Looks like we fail a cc_unittest. Seeing if I can reproduce it locally on > > linux. > > > > This doesn't have to do with the approximate containment but rather the > trimming > > in GrDrawContext. Here is the expected image: > > https://jsfiddle.net/00v08chz/ (this is actually with the fuzzy scissor test > > turned off) > > > > and here is the actual image: > > > > http://jsfiddle.net/yp73nvsm/1/ > > > > I can't really see a difference myself > > And here is the image with the fuzzy scissor test. I still can't see a > difference: > > https://jsfiddle.net/00v08chz/2/ Yeah there will be a lot of subtle differences after this change, due to us sending different geometry and interpolating between different points of local coords. Unless something looks obviously wrong I wouldn't worry too much about it. Can I help triage somewhere?
Message was sent while issue was closed.
Description was changed from ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a ========== to ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a ==========
The CQ bit was checked by csmartdalton@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a ========== to ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a Committed: https://skia.googlesource.com/skia/+/86de59f4a99b5f54be0483c60ff0335be55b2bdf ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/86de59f4a99b5f54be0483c60ff0335be55b2bdf
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2140253004/ by msarett@google.com. The reason for reverting is: I think this is still causing a test failure on Chrome windows bots. https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64....
Message was sent while issue was closed.
Description was changed from ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a Committed: https://skia.googlesource.com/skia/+/86de59f4a99b5f54be0483c60ff0335be55b2bdf ========== to ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a Committed: https://skia.googlesource.com/skia/+/86de59f4a99b5f54be0483c60ff0335be55b2bdf ==========
The CQ bit was checked by csmartdalton@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a Committed: https://skia.googlesource.com/skia/+/86de59f4a99b5f54be0483c60ff0335be55b2bdf ========== to ========== Pre-crop filled rects to avoid scissor Updates GrDrawContext to crop filled rects to the clip bounds before creating batches for them. Also adds clipping logic to ignore scissor when the draw falls completely inside. These two changes combined reduce API traffic and improve batching. In the future this can and should be improved by switching to floating point clip boundaries, thus allowing us to throw out non pixel aligned rectangle clips as well. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2132073002 Committed: https://skia.googlesource.com/skia/+/7969838702135b9f127bd738728da61bc49b050a Committed: https://skia.googlesource.com/skia/+/86de59f4a99b5f54be0483c60ff0335be55b2bdf Committed: https://skia.googlesource.com/skia/+/97f6cd5d0fc84dbed7cd8770b79695df83c69444 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/97f6cd5d0fc84dbed7cd8770b79695df83c69444
Message was sent while issue was closed.
On 2016/07/13 at 20:37:12, commit-bot wrote: > Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/97f6cd5d0fc84dbed7cd8770b79695df83c69444 Was this expected to cause a slight perf decrease, in particular on Nexus 10? https://perf.skia.org/#5338 https://perf.skia.org/cl/1224
Message was sent while issue was closed.
On 2016/07/19 19:58:26, jcgregorio wrote: > On 2016/07/13 at 20:37:12, commit-bot wrote: > > Committed patchset #6 (id:100001) as > https://skia.googlesource.com/skia/+/97f6cd5d0fc84dbed7cd8770b79695df83c69444 > > Was this expected to cause a slight perf decrease, in particular on Nexus 10? > > https://perf.skia.org/#5338 > > https://perf.skia.org/cl/1224 No, this comes as a surprise. I'm having a hard time understanding what the graph means that I'm looking at. is this a particular test or is it some sort of aggregate performance?
Message was sent while issue was closed.
On 2016/07/19 at 20:09:16, csmartdalton wrote: > On 2016/07/19 19:58:26, jcgregorio wrote: > > On 2016/07/13 at 20:37:12, commit-bot wrote: > > > Committed patchset #6 (id:100001) as > > https://skia.googlesource.com/skia/+/97f6cd5d0fc84dbed7cd8770b79695df83c69444 > > > > Was this expected to cause a slight perf decrease, in particular on Nexus 10? > > > > https://perf.skia.org/#5338 > > > > https://perf.skia.org/cl/1224 > > No, this comes as a surprise. I'm having a hard time understanding what the graph means that I'm looking at. is this a particular test or is it some sort of aggregate performance? This graph will be better, it's a selected set of traces that show the regression, with each line being an individual test. https://perf.skia.org/#5339 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
