Skia Filter Quality and Scaling Metrics

Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1686203002

Committed: https://skia.googlesource.com/skia/+/f57b3a6e4a002caf01378832cbd756c6c163a783

Committed: https://skia.googlesource.com/skia/+/983294f78f11159a7def7fd2ea0c12f911d17688

[ericrk, 2016-02-10 19:30:32]

Description was changed from

==========
Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
==========

to

==========
Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[ericrk, 2016-02-10 19:35:12]

ericrk@chromium.org changed reviewers:
+ bsalomon@google.com

[ericrk, 2016-02-10 19:35:12]

Adding some metrics to log scale factor and filter quality across draws.

[bsalomon, 2016-02-16 14:52:42]

https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h...
include/core/SkDevice.h:380: void logDrawScaleFactor(const SkMatrix&,
SkFilterQuality) const;
Maybe this should just be a static helper in SkGpuDevice.cpp since no other
subclass is using it (and it's only used in a couple targeted draw calls).

[ericrk, 2016-02-16 22:05:34]

https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h...
include/core/SkDevice.h:380: void logDrawScaleFactor(const SkMatrix&,
SkFilterQuality) const;
On 2016/02/16 at 14:52:42, bsalomon wrote:
> Maybe this should just be a static helper in SkGpuDevice.cpp since no other
subclass is using it (and it's only used in a couple targeted draw calls).

I'm using this in SkBitmapDevice as well - happy to refactor though if you have
something in mind.

[ericrk, 2016-02-24 00:40:20]

On 2016/02/16 22:05:34, ericrk wrote:
> https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h
> File include/core/SkDevice.h (right):
> 
>
https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h...
> include/core/SkDevice.h:380: void logDrawScaleFactor(const SkMatrix&,
> SkFilterQuality) const;
> On 2016/02/16 at 14:52:42, bsalomon wrote:
> > Maybe this should just be a static helper in SkGpuDevice.cpp since no other
> subclass is using it (and it's only used in a couple targeted draw calls).
> 
> I'm using this in SkBitmapDevice as well - happy to refactor though if you
have
> something in mind.

ping :D

[bsalomon, 2016-02-24 18:25:00]

https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h...
include/core/SkDevice.h:380: void logDrawScaleFactor(const SkMatrix&,
SkFilterQuality) const;
On 2016/02/16 22:05:33, ericrk wrote:
> On 2016/02/16 at 14:52:42, bsalomon wrote:
> > Maybe this should just be a static helper in SkGpuDevice.cpp since no other
> subclass is using it (and it's only used in a couple targeted draw calls).
> 
> I'm using this in SkBitmapDevice as well - happy to refactor though if you
have
> something in mind.

Gotcha.. didn't catch that. Need it be a class member or can it be static?

[ericrk, 2016-02-25 00:00:07]

Patchset #3 (id:40001) has been deleted

[ericrk, 2016-02-25 00:22:11]

Good point. Updated.

https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/1686203002/diff/20001/include/core/SkDevice.h...
include/core/SkDevice.h:380: void logDrawScaleFactor(const SkMatrix&,
SkFilterQuality) const;
On 2016/02/24 18:24:59, bsalomon wrote:
> On 2016/02/16 22:05:33, ericrk wrote:
> > On 2016/02/16 at 14:52:42, bsalomon wrote:
> > > Maybe this should just be a static helper in SkGpuDevice.cpp since no
other
> > subclass is using it (and it's only used in a couple targeted draw calls).
> > 
> > I'm using this in SkBitmapDevice as well - happy to refactor though if you
> have
> > something in mind.
> 
> Gotcha.. didn't catch that. Need it be a class member or can it be static?

made it static, good point.

https://codereview.chromium.org/1686203002/diff/60001/include/core/SkPostConf...
File include/core/SkPostConfig.h (right):

https://codereview.chromium.org/1686203002/diff/60001/include/core/SkPostConf...
include/core/SkPostConfig.h:349: #if defined(SK_HISTOGRAM_ENUMERATION) &&
defined(SK_HISTOGRAM_BOOLEAN)
Added this so we can avoid running unnecessary logic leading up to a histogram
log if the log fns themselves are not defined.

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkBitmapDevice...
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkBitmapDevice...
src/core/SkBitmapDevice.cpp:58: case kRGBA_F16_SkColorType:
I guess I inadvertently rebased - this is not from my change.

[bsalomon, 2016-02-25 01:51:53]

lgtm

[ericrk, 2016-02-25 18:59:04]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2016-02-25 18:59:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1686203002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1686203002/60001

[commit-bot: I haz the power, 2016-02-25 19:21:59]

Description was changed from

==========
Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/f57b3a6e4a002caf01378832cbd756c6c163a783
==========

[commit-bot: I haz the power, 2016-02-25 19:22:00]

Committed patchset #3 (id:60001) as
https://skia.googlesource.com/skia/+/f57b3a6e4a002caf01378832cbd756c6c163a783

[Ilya Sherman, 2016-02-25 22:08:52]

isherman@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2016-02-25 22:08:52]

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:456: SkASSERT(filterQuality != kNone_SkFilterQuality);
Why do you handle this case below, if you have this assertion here?

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:457: enum { kScaleFactorCount =
kLargeDownscale_ScaleFactor + 1 };
nit: Why not include this in the enum above?

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:494: enum { kFilterQualityCount = kHigh_SkFilterQuality +
1 };
This worries me a bit -- if the filter quality enum is ever updated to include
an additional element, how likely is it that this constant would be updated as
well?

[ericrk, 2016-02-25 22:53:11]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/1735423002/ by ericrk@chromium.org.

The reason for reverting is: reverting this to address Ilya's comments..

[ericrk, 2016-04-08 20:58:48]

Description was changed from

==========
Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/f57b3a6e4a002caf01378832cbd756c6c163a783
==========

to

==========
Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/f57b3a6e4a002caf01378832cbd756c6c163a783
==========

[ericrk, 2016-04-08 21:20:44]

Patchset #5 (id:100001) has been deleted

[ericrk, 2016-04-08 21:21:35]

Updated to address comments.

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:456: SkASSERT(filterQuality != kNone_SkFilterQuality);
On 2016/02/25 22:08:52, Ilya Sherman wrote:
> Why do you handle this case below, if you have this assertion here?

Didn't mean to have this. Removed.

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:457: enum { kScaleFactorCount =
kLargeDownscale_ScaleFactor + 1 };
On 2016/02/25 22:08:52, Ilya Sherman wrote:
> nit: Why not include this in the enum above?

Sure - style in Skia doesn't seem to put counts in enums - but they do put
"kLast" entries, so did this.

https://codereview.chromium.org/1686203002/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:494: enum { kFilterQualityCount = kHigh_SkFilterQuality +
1 };
On 2016/02/25 22:08:52, Ilya Sherman wrote:
> This worries me a bit -- if the filter quality enum is ever updated to include
> an additional element, how likely is it that this constant would be updated as
> well?

added a kLast_SkFilterQuality and used this instead.

[Ilya Sherman, 2016-04-09 06:18:07]

LGTM, thanks.

[ericrk, 2016-04-18 16:04:15]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-04-18 16:04:16]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1686203002/#ps120001
(title: "feedback")

[commit-bot: I haz the power, 2016-04-18 16:04:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1686203002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1686203002/120001

[commit-bot: I haz the power, 2016-04-18 16:14:02]

Description was changed from

==========
Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/f57b3a6e4a002caf01378832cbd756c6c163a783
==========

to

==========
Skia Filter Quality and Scaling Metrics

Adds histogram metrics to log the filter quality and scale factor of
each image draw. To make the data easier to consume, this is broken
down into a number of individual histograms:
- Filter quality across all draw calls
- Scale amount across all draw calls
- Scale amount per filter quality (4 histograms total)

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/f57b3a6e4a002caf01378832cbd756c6c163a783

Committed:
https://skia.googlesource.com/skia/+/983294f78f11159a7def7fd2ea0c12f911d17688
==========

[commit-bot: I haz the power, 2016-04-18 16:14:04]

Committed patchset #5 (id:120001) as
https://skia.googlesource.com/skia/+/983294f78f11159a7def7fd2ea0c12f911d17688

[Alexei Svitkine (slow), 2016-07-26 19:27:53]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-07-26 19:27:54]

Should this be reverted given it's causing issues in UMA pipelines?

Have we looked at what's the issue yet? (I think Eric was cc'd to the internal
thread on this.)

[ericrk, 2016-07-27 23:48:22]

On 2016/07/26 19:27:54, Alexei Svitkine (slow) wrote:
> Should this be reverted given it's causing issues in UMA pipelines?
> 
> Have we looked at what's the issue yet? (I think Eric was cc'd to the internal
> thread on this.)

I've sent out a CL to disable these (crrev.com/2185263002)- would rather not
revert at this point. I'll debug further.