Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1991413002:
When building circle blur profile evaluate kernel vertically once per column (Closed)
|
Created:
4 years, 7 months ago by bsalomon Modified:
4 years, 7 months ago Reviewers:
robertphillips CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@blursep Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionWhen building circle blur profile evaluate kernel vertically once per column
BUG=skia:5224
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1991413002
Committed: https://skia.googlesource.com/skia/+/82ad93c356d8c010e1260224c86ce6f74359a2da
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 7
Patch Set 3 : apply #Patch Set 4 : Fix float->double warning in msvs #Messages
Total messages: 21 (12 generated)
Description was changed from ========== When building circle blur profile evaluate kernel vertically once per column BUG=skia:5224 ========== to ========== When building circle blur profile evaluate kernel vertically once per column BUG=skia:5224 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
bsalomon@google.com changed reviewers: + robertphillips@google.com
This shaves another 20% off GM_blurcircles2. https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... File src/effects/GrCircleBlurFragmentProcessor.cpp (right): https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:146: if (x < -circleR || x > circleR) { I tried breaking this loop up into five parts: 1) x fully out of the circle to the left 2) x inside the circle, half kernel extends vertically outside the circle 3) x inside the circle, half kernel contained in y 4) 2 again 5) x fully outside the circle to the right However, I wasn't able to measure a speedup even with nullgpu.
lgtm + dox requests https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... File src/effects/GrCircleBlurFragmentProcessor.cpp (right): https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:139: at a points ? Maybe something more like: Create a table to store the ... ? Should the name of this method be changed? https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:167: // Conceptually combine the 2D Gaussian kernel positioned at (evalX, 0) with a circle centered at the origin with radius circleR. In reality, combine the look ups in the halfKernel table (for X) with look ups from the yKernelEvaluations (for Y). https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:207: Seems like this might need updating
https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... File src/effects/GrCircleBlurFragmentProcessor.cpp (right): https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:139: On 2016/05/20 14:44:18, robertphillips wrote: > at a points ? > > Maybe something more like: > > Create a table to store the ... > > ? > > Should the name of this method be changed? Done. https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:167: On 2016/05/20 14:44:19, robertphillips wrote: > // Conceptually combine the 2D Gaussian kernel positioned at (evalX, 0) with a > circle centered at the origin with radius circleR. In reality, combine the look > ups in the halfKernel table (for X) with look ups from the yKernelEvaluations > (for Y). Done. https://codereview.chromium.org/1991413002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:207: On 2016/05/20 14:44:19, robertphillips wrote: > Seems like this might need updating Done.
The CQ bit was checked by bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/1991413002/#ps40001 (title: "apply")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991413002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1991413002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/1991413002/#ps60001 (title: "Fix float->double warning in msvs")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1998113003 Patch 40001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by bsalomon@google.com
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1998113003 Patch 40001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by bsalomon@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991413002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1991413002/60001
Message was sent while issue was closed.
Description was changed from ========== When building circle blur profile evaluate kernel vertically once per column BUG=skia:5224 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== When building circle blur profile evaluate kernel vertically once per column BUG=skia:5224 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/82ad93c356d8c010e1260224c86ce6f74359a2da ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/82ad93c356d8c010e1260224c86ce6f74359a2da |
