Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168133002:
Compute exact bounds for SVGPathElement::getBBox() (Closed)
|
Created:
4 years, 5 months ago by f(malita) Modified:
4 years, 5 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCompute exact bounds for SVGPathElement::getBBox()
The current implementation relies on SkPath::getBounds(), which
computes a conservative result (includes control points).
Use SkPathOps::TightBounds() instead, for an exact answer.
BUG=230599
R=fs@opera.com,pdr@chromium.org,schenney@chromium.org,caryclark@google.com
Committed: https://crrev.com/8f4c8e6d2f1260d68f387b37f078457e4153bbb4
Cr-Commit-Position: refs/heads/master@{#407268}
Patch Set 1 #Patch Set 2 : minimize perf impact #Patch Set 3 : empty subpath workaround #
Total comments: 2
Patch Set 4 : review #
Messages
Total messages: 30 (20 generated)
The CQ bit was checked by fmalita@chromium.org 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by fmalita@chromium.org 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 checked by fmalita@chromium.org 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...
Description was changed from ========== Compute tight Path bounds. WIP BUG= ========== to ========== Compute exact bounds for SVGPathElement::getBBox() The current implementation relies on SkPath::getBounds(), which computes a conservative result (includes control points). Use SkPathOps::TightBounds() instead, for an exact answer. BUG=230599 R=fs@opera.com,pdr@chromium.org,schenney@chromium.org ==========
Description was changed from ========== Compute exact bounds for SVGPathElement::getBBox() The current implementation relies on SkPath::getBounds(), which computes a conservative result (includes control points). Use SkPathOps::TightBounds() instead, for an exact answer. BUG=230599 R=fs@opera.com,pdr@chromium.org,schenney@chromium.org ========== to ========== Compute exact bounds for SVGPathElement::getBBox() The current implementation relies on SkPath::getBounds(), which computes a conservative result (includes control points). Use SkPathOps::TightBounds() instead, for an exact answer. BUG=230599 R=fs@opera.com,pdr@chromium.org,schenney@chromium.org,caryclark@google.com ==========
fmalita@chromium.org changed reviewers: + caryclark@google.com, fs@opera.com, pdr@chromium.org, schenney@chromium.org
Ideally we should use exact bounds for everything (layout, objectBoundingBox(), strokeBoundingBox(), etc), but it becomes a question of how much we're willing to regress perf for correctness. PS#1 tries this approach, and running the blink_perf.svg suite locally shows some significant regressions - e.g. SvgCubics.html scores ~30% worse. SvgHitTesting.html shows a small improvement (~5%), and I suspect there are other rasterization benefits to tighter bounds (improved culling, reduced invalidation). Cary found an optimization to speed up the impl (https://bugs.chromium.org/p/skia/issues/detail?id=5553), so there's hope that the regression will diminish. In the meantime, I've updated the CL to only compute tight bounds for the getBBox() code path. This fixes the DOM API issue, but doesn't help with outlines for example. Not sure how I feel about landing the API-only fix: maybe it's better to sit on the CL until Cary's improvements land, and then re-evaluate using tight bounds everywhere? WDYT?
On 2016/07/21 at 18:52:05, fmalita wrote: > Ideally we should use exact bounds for everything (layout, objectBoundingBox(), strokeBoundingBox(), etc), but it becomes a question of how much we're willing to regress perf for correctness. > > PS#1 tries this approach, and running the blink_perf.svg suite locally shows some significant regressions - e.g. SvgCubics.html scores ~30% worse. SvgHitTesting.html shows a small improvement (~5%), and I suspect there are other rasterization benefits to tighter bounds (improved culling, reduced invalidation). Yeah, I was wonder about how much of a regression we'd be willing to swallow for tighter bounds. 30% does sound like a lot, but that test is likely a pretty "heavy" user. > Cary found an optimization to speed up the impl (https://bugs.chromium.org/p/skia/issues/detail?id=5553), so there's hope that the regression will diminish. > > In the meantime, I've updated the CL to only compute tight bounds for the getBBox() code path. This fixes the DOM API issue, but doesn't help with outlines for example. > > Not sure how I feel about landing the API-only fix: maybe it's better to sit on the CL until Cary's improvements land, and then re-evaluate using tight bounds everywhere? WDYT? Hmm, yeah, if it's easy to gauge the improvement it will bring, then maybe it'll be worth waiting a bit. If we land this, we'll need to un-dupe crbug.com/415023 because it's about objectBoundingBox in relation to paintservers. https://codereview.chromium.org/2168133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/Path.cpp (right): https://codereview.chromium.org/2168133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Path.cpp:128: DCHECK(boundsType == Path::BoundsType::Exact); (DCHECK_EQ? Or maybe that doesn't for enum class, not sure...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Awesome! What do you mean by "This fixes the DOM API issue, but doesn't help with outlines for example."?
On 2016/07/21 22:57:31, pdr. wrote: > What do you mean by "This fixes the DOM API issue, but doesn't help with > outlines for example."? The linked bug has two repros: one using JS getBBox(), and one using outlines. The most recent CL fixes the former, but not the latter. (We request exact bounds from SVGPathElement::getBBox(), which is used for servicing related SVG DOM APIs - https://www.w3.org/TR/SVG/types.html#__svg__SVGLocatable__getBBox. But for internal bounding box computations, PS#3 uses conservative bounds in order to avoid layout perf regressions => all dependent boxes are still conservative, including outlines.)
On 2016/07/21 at 23:19:57, fmalita wrote: > On 2016/07/21 22:57:31, pdr. wrote: > > What do you mean by "This fixes the DOM API issue, but doesn't help with > > outlines for example."? > > The linked bug has two repros: one using JS getBBox(), and one using outlines. The most recent CL fixes the former, but not the latter. > > (We request exact bounds from SVGPathElement::getBBox(), which is used for servicing related SVG DOM APIs - https://www.w3.org/TR/SVG/types.html#__svg__SVGLocatable__getBBox. > > But for internal bounding box computations, PS#3 uses conservative bounds in order to avoid layout perf regressions => all dependent boxes are still conservative, including outlines.) Makes sense, thanks. I think this patch is worth landing as-is ( % Fredrik's comment). I think it's useful to decouple the api fix from the perf patch which might roll in and out. LGTM
The CQ bit was checked by fmalita@chromium.org 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...
@fs are you on board with landing as suggested by pdr? https://codereview.chromium.org/2168133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/Path.cpp (right): https://codereview.chromium.org/2168133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Path.cpp:128: DCHECK(boundsType == Path::BoundsType::Exact); On 2016/07/21 19:21:40, fs wrote: > (DCHECK_EQ? Or maybe that doesn't for enum class, not sure...) Done.
On 2016/07/22 at 18:40:01, fmalita wrote: > @fs are you on board with landing as suggested by pdr? Absolutely. LGTM.
The CQ bit was unchecked by fmalita@chromium.org
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2168133002/#ps60001 (title: "review")
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 ========== Compute exact bounds for SVGPathElement::getBBox() The current implementation relies on SkPath::getBounds(), which computes a conservative result (includes control points). Use SkPathOps::TightBounds() instead, for an exact answer. BUG=230599 R=fs@opera.com,pdr@chromium.org,schenney@chromium.org,caryclark@google.com ========== to ========== Compute exact bounds for SVGPathElement::getBBox() The current implementation relies on SkPath::getBounds(), which computes a conservative result (includes control points). Use SkPathOps::TightBounds() instead, for an exact answer. BUG=230599 R=fs@opera.com,pdr@chromium.org,schenney@chromium.org,caryclark@google.com ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Compute exact bounds for SVGPathElement::getBBox() The current implementation relies on SkPath::getBounds(), which computes a conservative result (includes control points). Use SkPathOps::TightBounds() instead, for an exact answer. BUG=230599 R=fs@opera.com,pdr@chromium.org,schenney@chromium.org,caryclark@google.com ========== to ========== Compute exact bounds for SVGPathElement::getBBox() The current implementation relies on SkPath::getBounds(), which computes a conservative result (includes control points). Use SkPathOps::TightBounds() instead, for an exact answer. BUG=230599 R=fs@opera.com,pdr@chromium.org,schenney@chromium.org,caryclark@google.com Committed: https://crrev.com/8f4c8e6d2f1260d68f387b37f078457e4153bbb4 Cr-Commit-Position: refs/heads/master@{#407268} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8f4c8e6d2f1260d68f387b37f078457e4153bbb4 Cr-Commit-Position: refs/heads/master@{#407268} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
