Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2249463002:
Update blurred rrect mask filter creation method to also handle caching (Closed)
|
Created:
4 years, 4 months ago by robertphillips Modified:
4 years, 4 months ago Reviewers:
bsalomon CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionUpdate blurred rrect mask filter creation method to also handle caching
The caching of the mask will also be required for the vertex attribute path so I moved that into the helper. The parameters computed by ComputeBlurredRRectParams will be needed to construct the coverage geometry by the vertex attribute path so I moved that out of the helper.
This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws)
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249463002
Committed: https://skia.googlesource.com/skia/+/d39430d0574513438b5831947052353290204a06
Patch Set 1 #Patch Set 2 : Calm some compilers #
Total comments: 6
Patch Set 3 : Remove 3* in key computation #Messages
Total messages: 26 (18 generated)
Description was changed from ========== Update blurred rrect mask filter creation method to also handle caching This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws) BUG=skia: ========== to ========== Update blurred rrect mask filter creation method to also handle caching This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249463002 ==========
Description was changed from ========== Update blurred rrect mask filter creation method to also handle caching This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249463002 ========== to ========== Update blurred rrect mask filter creation method to also handle caching This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249463002 ==========
robertphillips@google.com changed reviewers: + bsalomon@google.com
Description was changed from ========== Update blurred rrect mask filter creation method to also handle caching This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249463002 ========== to ========== Update blurred rrect mask filter creation method to also handle caching The caching of the mask will also be required for the vertex attribute path so I moved that into the helper. The parameters computed by ComputeBlurredRRectParams will be needed to construct the coverage geometry by the vertex attribute path so I moved that out of the helper. This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249463002 ==========
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... src/effects/SkBlurMaskFilter.cpp:1023: builder[0] = 3*SkScalarCeilToInt(xformedSigma-1/6.0f); Why 3*? https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... src/effects/SkBlurMaskFilter.cpp:1029: builder[index++] = SkScalarCeilToInt(rrectToDraw.radii(c).fX); Is the idea to handle non-circular? Right now I think the x and y radii are the same right? Is ceiling chosen for any particular reason over round?
https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... src/effects/SkBlurMaskFilter.cpp:1023: builder[0] = 3*SkScalarCeilToInt(xformedSigma-1/6.0f); On 2016/08/15 13:55:23, bsalomon wrote: > Why 3*? We/SVG requires that we include 3*sigma of the shadow. https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... src/effects/SkBlurMaskFilter.cpp:1029: builder[index++] = SkScalarCeilToInt(rrectToDraw.radii(c).fX); On 2016/08/15 13:55:23, bsalomon wrote: > Is the idea to handle non-circular? Right now I think the x and y radii are the > same right? Is ceiling chosen for any particular reason over round? The code will soon be able to handle non-simple-circular rrects. If it is nine-patchable it will be nine-patched and cached. You are correct that right now the upstream code limits us to simple circular. No particular reason for ceil vs. round. Since they've been integerized they should land in the same place.
https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... src/effects/SkBlurMaskFilter.cpp:1023: builder[0] = 3*SkScalarCeilToInt(xformedSigma-1/6.0f); On 2016/08/15 17:41:37, robertphillips wrote: > On 2016/08/15 13:55:23, bsalomon wrote: > > Why 3*? > > We/SVG requires that we include 3*sigma of the shadow. Right but does it matter for the key?
https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/2249463002/diff/20001/src/effects/SkBlurMaskF... src/effects/SkBlurMaskFilter.cpp:1023: builder[0] = 3*SkScalarCeilToInt(xformedSigma-1/6.0f); On 2016/08/15 17:57:56, bsalomon wrote: > On 2016/08/15 17:41:37, robertphillips wrote: > > On 2016/08/15 13:55:23, bsalomon wrote: > > > Why 3*? > > > > We/SVG requires that we include 3*sigma of the shadow. > > Right but does it matter for the key? Nope - removed.
lgtm
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertphillips@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 ========== Update blurred rrect mask filter creation method to also handle caching The caching of the mask will also be required for the vertex attribute path so I moved that into the helper. The parameters computed by ComputeBlurredRRectParams will be needed to construct the coverage geometry by the vertex attribute path so I moved that out of the helper. This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249463002 ========== to ========== Update blurred rrect mask filter creation method to also handle caching The caching of the mask will also be required for the vertex attribute path so I moved that into the helper. The parameters computed by ComputeBlurredRRectParams will be needed to construct the coverage geometry by the vertex attribute path so I moved that out of the helper. This is split out of https://codereview.chromium.org/2245653002/ (Start using vertex attributes for nine-patch blurred rrect draws) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249463002 Committed: https://skia.googlesource.com/skia/+/d39430d0574513438b5831947052353290204a06 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/d39430d0574513438b5831947052353290204a06 |
