Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449583002:
Support rrect clips in analysis canvas (Closed)
|
Created:
4 years, 1 month ago by Ian Vollick Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport rrect clips in analysis canvas
If a canvas falls entirely within the rrect clip, it is as if it is not
clipped at all, so we early out. This change is in advance of using
tiles for masks (so that we use less memory for border radius masks).
BUG=590373
Committed: https://crrev.com/c752fed41772258d62cee790b06c21296b41e15e
Cr-Commit-Position: refs/heads/master@{#427689}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address reviewer feedback. #
Total comments: 4
Patch Set 3 : Address reviewer feedback. #
Messages
Total messages: 27 (16 generated)
The CQ bit was checked by vollick@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 ========== Support rrect clips in analysis canvas If a canvas falls entirely within the rrect clip, it is as if it is not clipped at all, so we early out. BUG=None ========== to ========== Support rrect clips in analysis canvas If a canvas falls entirely within the rrect clip, it is as if it is not clipped at all, so we early out. This change is in advance of using tiles for masks (so that we use less memory for border radius masks). BUG=590373 ==========
vollick@chromium.org changed reviewers: + mtklein@chromium.org
mtklein@chromium.org changed reviewers: + reed@google.com
+Mike
reed@google.com changed reviewers: + fmalita@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc File skia/ext/analysis_canvas.cc (right): https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... skia/ext/analysis_canvas.cc:413: SkRect toDeviceSpace(const SkVector& scale, SkMatrix::mapRectScaleTranslate()? (see below) https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... skia/ext/analysis_canvas.cc:459: toDeviceSpace(scale, translate, total_matrix.mapRectScaleTranslate(&ul_device_rect, ...)? https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... skia/ext/analysis_canvas.cc:465: toDeviceSpace(scale, translate, ditto https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... skia/ext/analysis_canvas.cc:470: SkRect ll_device_rect = toDeviceSpace( ditto https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... skia/ext/analysis_canvas.cc:476: SkRect lr_device_rect = toDeviceSpace( ditto https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... skia/ext/analysis_canvas.cc:482: SkRect bounding_device_rect = toDeviceSpace(scale, translate, bounding_rect); ditto https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... skia/ext/analysis_canvas.cc:496: !SkRect::Intersects(lr_device_rect, clip_device_rect); Not quite the same thing, but wouldn't it be sufficient to check containment against the inner rect (the equivalent of FloatRoundedRect::radiusCenterRect)? Then we only have to map/check 1 rect instead of 5. Or better yet, inverse-map clip_device_bounds and just use SkRRect::contains(const SkRect&)?
On 2016/10/25 13:26:25, f(malita) wrote: > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc > File skia/ext/analysis_canvas.cc (right): > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > skia/ext/analysis_canvas.cc:413: SkRect toDeviceSpace(const SkVector& scale, > SkMatrix::mapRectScaleTranslate()? > > (see below) > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > skia/ext/analysis_canvas.cc:459: toDeviceSpace(scale, translate, > total_matrix.mapRectScaleTranslate(&ul_device_rect, ...)? > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > skia/ext/analysis_canvas.cc:465: toDeviceSpace(scale, translate, > ditto > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > skia/ext/analysis_canvas.cc:470: SkRect ll_device_rect = toDeviceSpace( > ditto > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > skia/ext/analysis_canvas.cc:476: SkRect lr_device_rect = toDeviceSpace( > ditto > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > skia/ext/analysis_canvas.cc:482: SkRect bounding_device_rect = > toDeviceSpace(scale, translate, bounding_rect); > ditto > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > skia/ext/analysis_canvas.cc:496: !SkRect::Intersects(lr_device_rect, > clip_device_rect); > Not quite the same thing, but wouldn't it be sufficient to check containment > against the inner rect (the equivalent of FloatRoundedRect::radiusCenterRect)? > > Then we only have to map/check 1 rect instead of 5. > > Or better yet, inverse-map clip_device_bounds and just use > SkRRect::contains(const SkRect&)? Done. (Thanks, that's much nicer!)
The CQ bit was checked by vollick@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: This issue passed the CQ dry run.
On 2016/10/25 16:28:46, Ian Vollick wrote: > On 2016/10/25 13:26:25, f(malita) wrote: > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc > > File skia/ext/analysis_canvas.cc (right): > > > > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > > skia/ext/analysis_canvas.cc:413: SkRect toDeviceSpace(const SkVector& scale, > > SkMatrix::mapRectScaleTranslate()? > > > > (see below) > > > > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > > skia/ext/analysis_canvas.cc:459: toDeviceSpace(scale, translate, > > total_matrix.mapRectScaleTranslate(&ul_device_rect, ...)? > > > > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > > skia/ext/analysis_canvas.cc:465: toDeviceSpace(scale, translate, > > ditto > > > > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > > skia/ext/analysis_canvas.cc:470: SkRect ll_device_rect = toDeviceSpace( > > ditto > > > > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > > skia/ext/analysis_canvas.cc:476: SkRect lr_device_rect = toDeviceSpace( > > ditto > > > > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > > skia/ext/analysis_canvas.cc:482: SkRect bounding_device_rect = > > toDeviceSpace(scale, translate, bounding_rect); > > ditto > > > > > https://codereview.chromium.org/2449583002/diff/1/skia/ext/analysis_canvas.cc... > > skia/ext/analysis_canvas.cc:496: !SkRect::Intersects(lr_device_rect, > > clip_device_rect); > > Not quite the same thing, but wouldn't it be sufficient to check containment > > against the inner rect (the equivalent of FloatRoundedRect::radiusCenterRect)? > > > > Then we only have to map/check 1 rect instead of 5. > > > > Or better yet, inverse-map clip_device_bounds and just use > > SkRRect::contains(const SkRect&)? > > Done. (Thanks, that's much nicer!) ping.
LGTM https://codereview.chromium.org/2449583002/diff/20001/skia/ext/analysis_canva... File skia/ext/analysis_canvas.cc (right): https://codereview.chromium.org/2449583002/diff/20001/skia/ext/analysis_canva... skia/ext/analysis_canvas.cc:428: SkRect clip_rect = SkRect::MakeFromIRect(clip_device_bounds); nit: MakeFromIRect is deprecated, you can use Make(). https://codereview.chromium.org/2449583002/diff/20001/skia/ext/analysis_canva... skia/ext/analysis_canvas.cc:430: if (!clip_rect.isFinite()) { nit: Is this check needed? I think the contains test below would cover this case too.
https://codereview.chromium.org/2449583002/diff/20001/skia/ext/analysis_canva... File skia/ext/analysis_canvas.cc (right): https://codereview.chromium.org/2449583002/diff/20001/skia/ext/analysis_canva... skia/ext/analysis_canvas.cc:428: SkRect clip_rect = SkRect::MakeFromIRect(clip_device_bounds); On 2016/10/26 12:24:24, f(malita) wrote: > nit: MakeFromIRect is deprecated, you can use Make(). Done. https://codereview.chromium.org/2449583002/diff/20001/skia/ext/analysis_canva... skia/ext/analysis_canvas.cc:430: if (!clip_rect.isFinite()) { On 2016/10/26 12:24:24, f(malita) wrote: > nit: Is this check needed? I think the contains test below would cover this > case too. Nope, not necessary. Removed.
The CQ bit was checked by vollick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2449583002/#ps40001 (title: "Address reviewer feedback.")
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 ========== Support rrect clips in analysis canvas If a canvas falls entirely within the rrect clip, it is as if it is not clipped at all, so we early out. This change is in advance of using tiles for masks (so that we use less memory for border radius masks). BUG=590373 ========== to ========== Support rrect clips in analysis canvas If a canvas falls entirely within the rrect clip, it is as if it is not clipped at all, so we early out. This change is in advance of using tiles for masks (so that we use less memory for border radius masks). BUG=590373 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Support rrect clips in analysis canvas If a canvas falls entirely within the rrect clip, it is as if it is not clipped at all, so we early out. This change is in advance of using tiles for masks (so that we use less memory for border radius masks). BUG=590373 ========== to ========== Support rrect clips in analysis canvas If a canvas falls entirely within the rrect clip, it is as if it is not clipped at all, so we early out. This change is in advance of using tiles for masks (so that we use less memory for border radius masks). BUG=590373 Committed: https://crrev.com/c752fed41772258d62cee790b06c21296b41e15e Cr-Commit-Position: refs/heads/master@{#427689} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c752fed41772258d62cee790b06c21296b41e15e Cr-Commit-Position: refs/heads/master@{#427689} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
