Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
708073002:
Use size of SkRect as the key for blur mask cache (Closed)
DescriptionUse size of SkRect as the key for blur mask cache
BUG=429409
Committed: https://skia.googlesource.com/skia/+/69469b570ceb8a5ab7f64778d1b128cd5c472e73
Patch Set 1 #Patch Set 2 : fix GM blur2rects #Patch Set 3 : fix fractional rects #
Messages
Total messages: 19 (2 generated)
qiankun.miao@intel.com changed reviewers: + reed@google.com
PTAL.
What bug is this fixing? Why is it correct to ignore the phase of the outer rect?
On 2014/11/07 14:49:32, reed1 wrote:
> What bug is this fixing? Why is it correct to ignore the phase of the outer
> rect?
1. For a box-shadow effect, mask will be computed several times with differenct
rects but same size. This CL avoids such calculation.
Try simple case as follow, log rects passed to find_cached_rects():
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<style>
div
{
width:1000px;
height:800px;
box-shadow:0 -170px 160px -100px rgba(0,0,0,0.6) inset, 0 170px 160px -100px
inset;
}
</style>
</head>
<body>
<div>
</div>
</body>
</html>
2. Result fImage of SkMask only depends on size of rects and relative position
of outer and inner rect.
#include "SkBlurMaskFilter.h"
static void draw_blur(SkCanvas* canvas) {
SkPaint paint;
paint.setMaskFilter(SkBlurMaskFilter::Create(kNormal_SkBlurStyle,
2.3f))->unref();
SkRect outer = SkRect::MakeXYWH(10.1f, 10.1f, 100.3f, 100.3f);
SkRect inner = SkRect::MakeXYWH(20, 20, 80, 80);
SkPath path;
path.addRect(outer, SkPath::kCW_Direction);
path.addRect(inner, SkPath::kCCW_Direction);
// path.offset(0.3f, 0.3f);
canvas->drawPath(path, paint);
}
This will generate two different masks depending on if we execute the
path.offset line. I think it is incorrect to ignore the phase of the outer rect.
#include "SkBlurMaskFilter.h"
static void draw_blur(SkCanvas* canvas) {
SkPaint paint;
paint.setMaskFilter(SkBlurMaskFilter::Create(kNormal_SkBlurStyle,
2.3f))->unref();
SkRect outer = SkRect::MakeXYWH(10.1f, 10.1f, 100.3f, 100.3f);
SkRect inner = SkRect::MakeXYWH(20, 20, 80, 80);
SkPath path;
path.addRect(outer, SkPath::kCW_Direction);
path.addRect(inner, SkPath::kCCW_Direction);
// path.offset(0.3f, 0.3f);
// canvas.translate(-0.3f, -0.3f);
canvas->drawPath(path, paint);
}
Add canvas.translate after path.offset will generate the same masks.
draw_rects_into_mask() does similar thing. So this CL should be correct to
ignore the phase of the outer rect.
On 2014/11/13 07:53:37, qiankun wrote:
> #include "SkBlurMaskFilter.h"
> static void draw_blur(SkCanvas* canvas) {
> SkPaint paint;
>
> paint.setMaskFilter(SkBlurMaskFilter::Create(kNormal_SkBlurStyle,
> 2.3f))->unref();
>
> SkRect outer = SkRect::MakeXYWH(10.1f, 10.1f, 100.3f, 100.3f);
> SkRect inner = SkRect::MakeXYWH(20, 20, 80, 80);
> SkPath path;
> path.addRect(outer, SkPath::kCW_Direction);
> path.addRect(inner, SkPath::kCCW_Direction);
>
> // path.offset(0.3f, 0.3f);
> // canvas.translate(-0.3f, -0.3f);
> canvas->drawPath(path, paint);
> }
>
> Add canvas.translate after path.offset will generate the same masks.
> draw_rects_into_mask() does similar thing. So this CL should be correct to
> ignore the phase of the outer rect.
Not sure what you mean. In the first code snippet, this CL will treat both paths
draws (w/ and w/o the offset) as the same cache key, which will have it return
the same mask for both, even though they should be different, hence I think this
CL is wrong.
On 2014/11/13 18:43:53, reed1 wrote:
> On 2014/11/13 07:53:37, qiankun wrote:
> > #include "SkBlurMaskFilter.h"
> > static void draw_blur(SkCanvas* canvas) {
> > SkPaint paint;
> >
> > paint.setMaskFilter(SkBlurMaskFilter::Create(kNormal_SkBlurStyle,
> > 2.3f))->unref();
> >
> > SkRect outer = SkRect::MakeXYWH(10.1f, 10.1f, 100.3f, 100.3f);
> > SkRect inner = SkRect::MakeXYWH(20, 20, 80, 80);
> > SkPath path;
> > path.addRect(outer, SkPath::kCW_Direction);
> > path.addRect(inner, SkPath::kCCW_Direction);
> >
> > // path.offset(0.3f, 0.3f);
> > // canvas.translate(-0.3f, -0.3f);
> > canvas->drawPath(path, paint);
> > }
> >
> > Add canvas.translate after path.offset will generate the same masks.
> > draw_rects_into_mask() does similar thing. So this CL should be correct to
> > ignore the phase of the outer rect.
>
> Not sure what you mean. In the first code snippet, this CL will treat both
paths
> draws (w/ and w/o the offset) as the same cache key, which will have it return
> the same mask for both, even though they should be different, hence I think
this
> CL is wrong.
In Skia's code, mask is generated in SkBlurMaskFilter::draw_rects_into_mask().
That function creates same mask pixels for rects with same size and same
relative position between outer and inner rects though phase of the outer rect
is different. That's because draw_rects_into_mask() call
canvas.translate(-SkIntToScalar(mask->fBounds.left()),
-SkIntToScalar(mask->fBounds.top())); to ignore phase of the outer rect. In your
code snippet, if you call canvas.translate() after path.offset() (same behavior
as draw_rects_into_mask() do), both paths (w/ and w/o the offset) will have the
same mask pixels.
https://codereview.chromium.org/728863002/ This new GM "blur2rects" show the current behavior, where the two drawing versions are slightly different, which is correct as the path is shifted slightly to the right (relative to the pixel boundaries). With the current CL applied, this subtle difference is lost, which is incorrect I believe.
The two drawing versions are slightly different due to precision. Fixed this issue using smallR as the key.
I have updated the gm slightly, and now it may more clearly show that this CL misses the subtle difference between the two draws. The blur code does (correctly) notice the slight phase-shift in the outer rect, and so we can't throw that information away when we try to cache it.
On 2014/11/14 20:20:07, reed1 wrote: > I have updated the gm slightly, and now it may more clearly show that this CL > misses the subtle difference between the two draws. > > The blur code does (correctly) notice the slight phase-shift in the outer rect, > and so we can't throw that information away when we try to cache it. After this GM update, both patch set 1 and set 2 can pass GM. In my opinion, patch set 2 should be correct.
Updated this CL to take rects with fractional boundary into account.
hi reed, could you take a look at this update?
Ping. Any comments about this CL?
The CQ bit was checked by reed@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/708073002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/69469b570ceb8a5ab7f64778d1b128cd5c472e73 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
