Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1996653002:
Make circle blur profile computation separable (Closed)
|
Created:
4 years, 7 months ago by bsalomon Modified:
4 years, 7 months ago Reviewers:
robertphillips CC:
reviews_skia.org Base URL:
https://chromium.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionMake circle blur profile computation separable
BUG=skia:5224
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1996653002
Committed: https://skia.googlesource.com/skia/+/b525721907fa56fd20682116f7645b4d0a861b78
Patch Set 1 #Patch Set 2 : tiny #
Total comments: 16
Patch Set 3 : Address comments #Patch Set 4 : fix double to float warning on msvs #Messages
Total messages: 17 (9 generated)
Description was changed from ========== Make circle blur profile computation separable BUG=skia: ========== to ========== Make circle blur profile computation separable BUG=skia: 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
Brings the median time for GM_blurcircles2 bench down to 1.5ms from 7.8ms. There are still more improvements that could be made here.
Description was changed from ========== Make circle blur profile computation separable BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make circle blur profile computation separable BUG=skia:5224 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... File src/effects/GrCircleBlurFragmentProcessor.cpp (right): https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:114: Gaussian ? https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:116: // steps. // The half-kernal is "normalized" to 0.5. ? https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:129: } 0.0f - to be consistent ? https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:143: int halfKernelSize) { one -> on ? https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:151: if (y < 0) { Won't this return a negative value ? https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:166: Can this be one loop ? https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:182: } we were -> we ? https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:202: // This method creates a table for half the Gaussian and a matching summed area table. It then creates the profile curve by stepping along in X and, for each point, using the summed area table to evaluate the y contribution (bounded by the size of the circle) and multiplying that by a look up into the Gaussian table. ?
lgtm
https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... File src/effects/GrCircleBlurFragmentProcessor.cpp (right): https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:114: On 2016/05/19 20:38:16, robertphillips wrote: > Gaussian ? Done. https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:116: // steps. On 2016/05/19 20:38:16, robertphillips wrote: > // The half-kernal is "normalized" to 0.5. > ? Done. https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:129: } On 2016/05/19 20:38:16, robertphillips wrote: > 0.0f - to be consistent ? Done. https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:143: int halfKernelSize) { On 2016/05/19 20:38:16, robertphillips wrote: > one -> on ? Done. https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:151: if (y < 0) { On 2016/05/19 20:38:16, robertphillips wrote: > Won't this return a negative value ? No, y has a lower bound of -0.5. https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:166: On 2016/05/19 20:38:16, robertphillips wrote: > Can this be one loop ? I tried but I didn't want to add logic to properly index halfKernel. https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:182: } On 2016/05/19 20:38:16, robertphillips wrote: > we were -> we ? Done. https://codereview.chromium.org/1996653002/diff/20001/src/effects/GrCircleBlu... src/effects/GrCircleBlurFragmentProcessor.cpp:202: On 2016/05/19 20:38:16, robertphillips wrote: > // This method creates a table for half the Gaussian and a matching summed area > table. It then creates the profile curve by stepping along in X and, for each > point, using the summed area table to evaluate the y contribution (bounded by > the size of the circle) and multiplying that by a look up into the Gaussian > table. > > ? 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/1996653002/#ps40001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996653002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996653002/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...)
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/1996653002/#ps60001 (title: "fix double to float warning on msvs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996653002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996653002/60001
Message was sent while issue was closed.
Description was changed from ========== Make circle blur profile computation separable BUG=skia:5224 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make circle blur profile computation separable 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/+/b525721907fa56fd20682116f7645b4d0a861b78 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/b525721907fa56fd20682116f7645b4d0a861b78 |
