Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(187)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 52793005: Add gms and benchmarks for drawing blurry round rects. (Closed)

Created:
7 years, 1 month ago by scroggo
Modified:
7 years, 1 month ago
Reviewers:
robertphillips
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Add gms and benchmarks for drawing blurry round rects. Further changes (https://codereview.chromium.org/48623006) will change the speed at which the bench draws and the drawing of the gm (the gm change is small). One of these round rects causes slow drawing in a webpage that we have observed. BUG=https://b.corp.google.com/issue?id=11174385 Committed: https://code.google.com/p/skia/source/detail?r=12133

Patch Set 1 #

Patch Set 2 : Use getLoops() #

Patch Set 3 : Add scaling to the gms. #

Patch Set 4 : More test cases #

Total comments: 37
Unified diffs Side-by-side diffs Delta from patch set Stats (+322 lines, -0 lines) Patch
A bench/BlurRoundRectBench.cpp View 1 1 chunk +109 lines, -0 lines 14 comments Download
A gm/blurroundrect.cpp View 1 2 3 1 chunk +211 lines, -0 lines 23 comments Download
M gyp/bench.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M gyp/gmslides.gypi View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
scroggo
7 years, 1 month ago (2013-11-01 22:05:00 UTC) #1
scroggo
Committed patchset #4 manually as r12133 (presubmit successful).
7 years, 1 month ago (2013-11-05 15:57:25 UTC) #2
robertphillips
lgtm + questions & suggestions https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench.cpp File bench/BlurRoundRectBench.cpp (right): https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench.cpp#newcode21 bench/BlurRoundRectBench.cpp:21: // Large blurred RR ...
7 years, 1 month ago (2013-11-05 16:31:03 UTC) #3
scroggo
7 years, 1 month ago (2013-11-05 20:45:41 UTC) #4
Message was sent while issue was closed. 
Robert, I believe I have addressed all of your comments in
https://codereview.chromium.org/59983004. Please take a look there.

https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench...
File bench/BlurRoundRectBench.cpp (right):

https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench...
bench/BlurRoundRectBench.cpp:21: 
On 2013/11/05 16:31:03, robertphillips wrote:
> // Large blurred RR appear frequently on web pages. This benchmark measures
our
> performance in this case.

Done.

https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench...
bench/BlurRoundRectBench.cpp:56: virtual void onDraw(SkCanvas* canvas)
SK_OVERRIDE {
On 2013/11/05 16:31:03, robertphillips wrote:
> It seems like the paint/looper/etc. should be created once outside of the
loop?
> That is a lot of allocation & deallocation we're timing.

Done.

https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench...
bench/BlurRoundRectBench.cpp:61: info.fFlagsMask = 0;
On 2013/11/05 16:31:03, robertphillips wrote:
> 40?

Done.

https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench...
bench/BlurRoundRectBench.cpp:70: paint->setMaskFilter(maskFilter)->unref();
On 2013/11/05 16:31:03, robertphillips wrote:
> 4279308561?

These were taken from the SKP. They also break warnings as errors, so I've
switched to using SK_Colors in another patch.

https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench...
bench/BlurRoundRectBench.cpp:73: paint->setColorFilter(colorFilter)->unref();
On 2013/11/05 16:31:03, robertphillips wrote:
> 4278190080?

same

https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench...
bench/BlurRoundRectBench.cpp:84: paint.setLooper(looper)->unref();
On 2013/11/05 16:31:03, robertphillips wrote:
> 4293848814?

same

https://codereview.chromium.org/52793005/diff/130001/bench/BlurRoundRectBench...
bench/BlurRoundRectBench.cpp:94: private:
On 2013/11/05 16:31:03, robertphillips wrote:
> OCD me says these should be lined up.

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp
File gm/blurroundrect.cpp (right):

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:50: radii[3].set(SkIntToScalar(llX), SkIntToScalar(llY));
On 2013/11/05 16:31:03, robertphillips wrote:
> Why not do this for the bench?

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:72: 
On 2013/11/05 16:31:03, robertphillips wrote:
> Doesn't seem like this virtual is buying us much.

Removed.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:82: private:
On 2013/11/05 16:31:03, robertphillips wrote:
> line these up?

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:115: info.fFlagsMask = 0;
On 2013/11/05 16:31:03, robertphillips wrote:
> 40?

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:124: paint->setMaskFilter(maskFilter)->unref();
On 2013/11/05 16:31:03, robertphillips wrote:
> hexify?

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:127: paint->setColorFilter(colorFilter)->unref();
On 2013/11/05 16:31:03, robertphillips wrote:
> hexify?

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:137: paint.setLooper(looper)->unref();
On 2013/11/05 16:31:03, robertphillips wrote:
> hexify?

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:147: 
On 2013/11/05 16:31:03, robertphillips wrote:
> // Simpler blurred RR test case where all the radii are the same.

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:158: // variables that are changing.
On 2013/11/05 16:31:03, robertphillips wrote:
> overlength

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:172: SkMaskFilter* filter =
SkBlurMaskFilter::Create(fBlurRadius,
On 2013/11/05 16:31:03, robertphillips wrote:
> indent?

Done.

https://codereview.chromium.org/52793005/diff/130001/gm/blurroundrect.cpp#new...
gm/blurroundrect.cpp:184: 
On 2013/11/05 16:31:03, robertphillips wrote:
> This is going to generate a bunch more images. Can these all be atlased into
one
> or two GM images?

Done.

Powered by Google App Engine
This is Rietveld 408576698