add draw filter option to picture_bench

add draw filter option to picture_bench

Option allows adding a draw filter option per type
or for all types. All SkPaint flags may be filtered,
plus disabling blur and setting the hint level.
Committed: https://code.google.com/p/skia/source/detail?r=6318

[Leon, 2012-11-06 17:05:32]

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:82: canvas->setDrawFilter(new
FlagsDrawFilter(fDrawFilters))->unref();
This should go in setupCanvas(int width, int height), since TiledPictureRenderer
calls that version, bypassing this one.

Also, 'new' should be 'SkNEW_ARGS'

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:116: SkString getConfigName() {
Should we add the drawfilters to the config name?

[caryclark1, 2012-11-06 19:25:57]

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:82: canvas->setDrawFilter(new
FlagsDrawFilter(fDrawFilters))->unref();
On 2012/11/06 17:05:32, scroggo-work wrote:
> This should go in setupCanvas(int width, int height), since
TiledPictureRenderer
> calls that version, bypassing this one.
> 
> Also, 'new' should be 'SkNEW_ARGS'

Done.

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:116: SkString getConfigName() {
On 2012/11/06 17:05:32, scroggo-work wrote:
> Should we add the drawfilters to the config name?

Deferring to you and Eric to determine what changes to make to the config name.

[EricB, 2012-11-06 19:34:37]

Looks good overall, but we need to include everything that we're filtering in
the bench data.

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:116: SkString getConfigName() {
On 2012/11/06 19:25:57, caryclark1 wrote:
> On 2012/11/06 17:05:32, scroggo-work wrote:
> > Should we add the drawfilters to the config name?
> 
> Deferring to you and Eric to determine what changes to make to the config
name.

We definitely want to differentiate in the bench data where we've turned certain
settings off.  It seems that this is the place to do so.  Something to the
effect of:

for each filter {
  config.append("skip_%s", filter);
}

Does that sound right?

https://codereview.appspot.com/6816092/diff/7001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6816092/diff/7001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:70: }else if
(PictureRenderer::kSlightHinting_DrawFilterFlag & fFlags[t]) {
nit: spacing

[Leon, 2012-11-06 19:43:29]

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:116: SkString getConfigName() {
On 2012/11/06 19:34:37, EricB wrote:
> On 2012/11/06 19:25:57, caryclark1 wrote:
> > On 2012/11/06 17:05:32, scroggo-work wrote:
> > > Should we add the drawfilters to the config name?
> > 
> > Deferring to you and Eric to determine what changes to make to the config
> name.
> 
> We definitely want to differentiate in the bench data where we've turned
certain
> settings off.  It seems that this is the place to do so.  Something to the
> effect of:
> 
> for each filter {
>   config.append("skip_%s", filter);
> }
> 
> Does that sound right?

Sounds appropriate to me.

https://codereview.appspot.com/6816092/diff/8005/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6816092/diff/8005/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:88: return setUpFilter(canvas, fDrawFilters);
setUpFilter does not need to be called here, since it will be called by the
other setupCanvas.

[caryclark1, 2012-11-06 21:01:06]

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6816092/diff/4001/tools/PictureRenderer.h#newc...
tools/PictureRenderer.h:116: SkString getConfigName() {
On 2012/11/06 19:43:29, scroggo-work wrote:
> On 2012/11/06 19:34:37, EricB wrote:
> > On 2012/11/06 19:25:57, caryclark1 wrote:
> > > On 2012/11/06 17:05:32, scroggo-work wrote:
> > > > Should we add the drawfilters to the config name?
> > > 
> > > Deferring to you and Eric to determine what changes to make to the config
> > name.
> > 
> > We definitely want to differentiate in the bench data where we've turned
> certain
> > settings off.  It seems that this is the place to do so.  Something to the
> > effect of:
> > 
> > for each filter {
> >   config.append("skip_%s", filter);
> > }
> > 
> > Does that sound right?
> 
> Sounds appropriate to me.

Done.

https://codereview.appspot.com/6816092/diff/7001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6816092/diff/7001/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:70: }else if
(PictureRenderer::kSlightHinting_DrawFilterFlag & fFlags[t]) {
On 2012/11/06 19:34:37, EricB wrote:
> nit: spacing

Done.

https://codereview.appspot.com/6816092/diff/8005/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6816092/diff/8005/tools/PictureRenderer.cpp#ne...
tools/PictureRenderer.cpp:88: return setUpFilter(canvas, fDrawFilters);
On 2012/11/06 19:43:29, scroggo-work wrote:
> setUpFilter does not need to be called here, since it will be called by the
> other setupCanvas.

Done.

[Leon, 2012-11-06 21:10:33]

lgtm

[EricB, 2012-11-06 21:18:58]

On 2012/11/06 21:10:33, scroggo-work wrote:
> lgtm

LGTM