Add usage metrics for display and image color profiles

Add usage metrics for display and image color profiles

This change adds new UMA histograms to track the use of color
profiles. This will allow us to study the prevalence of various
uses and error conditions related to the retrieval, decoding
and application of color profiles.

The new metrics track display device color profiles as well as
ICC profiles embedded in image files. This information will allow
us to make more enlightened decisions for mitigating issues like
crbug.com/562951

BUG=564080
R=noel@chromium.org, rkaplow@chromium.org

[Justin Novosad, 2015-12-01 17:38:20]

PTAL

[Justin Novosad, 2015-12-02 17:30:02]

Noel: I added a bucket for tracking iccV4 profiles. Because the histogram
buckets are mutually exclusive, I had to introduce a slight change in logic to
make it happen: I test for v4 and abort if true, which means we stop before
getting to the qcms_profile_from_memory call when a v4 profile is encountered. 
I think this is harmless, but I just wanted to point it out.

[rkaplow, 2015-12-02 21:56:42]

https://codereview.chromium.org/1488133002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1488133002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4682: +<histogram
name="ColorManagement.JPEGColorProfileStatus"
this could be made more concise by using the suffix notation, i.e.

histogram ColorManagement.ColorProfileStatus
with suffixes JPEG, PNG, etc. Can see examples of suffixes in this file.

This makes it easier to add new histograms as well with much less boilerplate.

https://codereview.chromium.org/1488133002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4684: +  <owner>junov@chromium.org
noel@chromium.org</owner>
these should be seperate lines (i.e. seperate owner tag per)

[Justin Novosad, 2015-12-02 22:15:43]

https://codereview.chromium.org/1488133002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1488133002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4682: +<histogram
name="ColorManagement.JPEGColorProfileStatus"
On 2015/12/02 21:56:42, rkaplow wrote:
> this could be made more concise by using the suffix notation, i.e.
> 
> histogram ColorManagement.ColorProfileStatus
> with suffixes JPEG, PNG, etc. Can see examples of suffixes in this file.
> 
> This makes it easier to add new histograms as well with much less boilerplate.

Done.

https://codereview.chromium.org/1488133002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4684: +  <owner>junov@chromium.org
noel@chromium.org</owner>
On 2015/12/02 21:56:42, rkaplow wrote:
> these should be seperate lines (i.e. seperate owner tag per)

Done.

[rkaplow, 2015-12-03 15:42:42]

lgtm

https://codereview.chromium.org/1488133002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1488133002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:79025: +  <suffix name="Display"
label="Color profiles from display devices."/>
ColorProfiles to be consistant?

[Justin Novosad, 2015-12-10 16:13:56]

noel ping

[Noel Gordon, 2015-12-15 06:10:34]

On 2015/12/10 16:13:56, Justin Novosad wrote:
> noel ping

Polish last week.  Added notes on the bug [1] to assess the various options.

[1] https://code.google.com/p/chromium/issues/detail?id=564080

[Noel Gordon, 2015-12-23 15:28:49]

Done blink gardening, and never an idle moment. Got my POC up-to-date, and next
I looked here to see how things would interwork together.

We could move this into it's own class, like the Canvas and Bitmap stats, but I
think this is fine start for now ...

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:255: status =
ColorProfileIsV4;
We don't seem to actually read / use the profile in this case :)  Any reason?

Perhaps we should just record the version of the display profile in another
stat, using qcms_profile_get_version(m_outputDeviceProfile) and record that if
needed?

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:259: status =
ColorProfileFailedDecode;
ColorProfileFailedDecode -> ColorProfileInvalidContent

the profile has a size, but the content is invalid in some way, unsupported, bad
tags, bad PCS ...

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:262: status =
ColorProfileIsBogus;
ColorProfileIsBogus -> ColorProfileInvalidProfile

let's not use bogus :) ... the profile has a size and valid content, but we
can't use it as a display profile for reasons known to us (bogosity, bad
version, or other factors we might add later, etc).

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:269: status =
ColorProfileNone;
ColorProfileNone -> ColorProfileEmpty

the profile has no size, or is empty.

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:54:
ColorProfileNone = 0,
I think we an reduce the number of entries here.  See the renaming suggestions
elsewhere.

ColorProfileEmpty (no size)
ColorProfileInvalidContent (size but invalid content)
ColorProfileInvalidProfile (valid size and content but bogus / unusable in some
way)
ColorProfileSuccess (valid size, content, usable)
ColorProfileSuccessEmbeddedsRGB (special case, embedded profiles).

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:231: static
bool isICCv4(const char* profileData, unsigned profileLength)
This can go, I think.  Note we have an API to get the version

  unsigned major_profile_version =
qcms_profile_get_version(m_outputDeviceProfile) >> 8;

which can be used once we have read a profile, and we should perhaps record that
profile version in another stat if needed?

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:215:
status = ColorProfileNone;
ColorProfileNone -> ColorProfileEmpty

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:223:
status = ColorProfileIsCorrupted;
ColorProfileIsCorrupted -> ColorProfileInvalidContent

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:224:
else if (ImageDecoder::isICCv4(profileData, profileLength))
We can remove this, and figure out if we need to record version in another stat.

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:227:
status = ColorProfileIsNotRGB;
ColorProfileIsNotRGB -> ColorProfileInvalidContent

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:229:
status = ColorProfileIsNotAnInputProfile;
ColorProfileIsNotAnInputProfile -> ColorProfileInvalidContent

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:652:
status = ColorProfileFailedDecode;
ColorProfileFailedDecode -> ColorProfileInvalidContent

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:172:
*status = ColorProfileFailedDecode;
ColorProfileFailedDecode -> ColorProfileInvalidContent

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:220:
status = ColorProfileSuccess_PNGsRGB;
ColorProfileSuccess_PNGsRGB -> ColorProfileSuccessEmbeddedsRGB

I'm thinking the following will be needed later

ColorProfileSuccessExifsRGB
ColorProfileSuccessExifAdobeRGB1998
ColorProfileSuccessEmbeddedAdobeRGB1998

Not sure about adding the image type to these.  The stat name includes PNG,
yeah?  /me looking at line 311 diff-right.

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:233:
status = ColorProfileNone;
ColorProfileNone -> ColorProfileEmpty

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:239:
char* profileData = reinterpret_cast<char*>(profile);
239-247, same as per the JPEG decoder.  Lump failures in
ColorProfileInvalidContent, remove the version (V4) bit.

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:247:
status = ColorProfileNone;
ColorProfileEmpty

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:332:
status = ColorProfileFailedDecode;
ColorProfileFailedDecode -> ColorProfileInvalidContent

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:355:
return ColorProfileNotFound;
ColorProfileNotFound -> ColorProfileEmpty

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:362:
ColorProfileStatus status = ColorProfileSuccess;
362-370, same as per the JPEG decoder.

https://codereview.chromium.org/1488133002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1488133002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:56807: +  <int value="0" label="Has no
color profile"/>
My renaming suggestions mean these will need updating.

https://codereview.chromium.org/1488133002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:79025: +  <suffix name="Display"
label="Color profiles from display devices."/>
On 2015/12/03 15:42:42, rkaplow - OOO till 2016 wrote:
> ColorProfiles to be consistant?

Perhaps ...

Display device color profile.
JPEG image color profile.
WEPB image color profile.
PNG image color profile.

(Eventually "Video frame color profile" and so on).

[Noel Gordon, 2015-12-23 15:32:04]

Description was changed from

==========
Adding usage metrics for display and image color profiles

This change adds new UMA histograms to track the use of color
profiles. This will allow us to study the prevalence of various
uses and error conditions related to the retrieval, decoding
and application of color profiles.

The new metrics track display device color profiles as well as
ICC profiles embedded in image files. This information will allow
us to make more enlightened decisions for mitigating issues like
crbug.com/562951

BUG=564080
R=noel@chromium.org, rkaplow@chromium.org
==========

to

==========
Add usage metrics for display and image color profiles

This change adds new UMA histograms to track the use of color
profiles. This will allow us to study the prevalence of various
uses and error conditions related to the retrieval, decoding
and application of color profiles.

The new metrics track display device color profiles as well as
ICC profiles embedded in image files. This information will allow
us to make more enlightened decisions for mitigating issues like
crbug.com/562951

BUG=564080
R=noel@chromium.org, rkaplow@chromium.org
==========

[Justin Novosad, 2015-12-23 15:58:10]

Thanks for the detailed feedback.  Will look into this in the new year.

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/1488133002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:255: status =
ColorProfileIsV4;
On 2015/12/23 15:28:48, noel gordon wrote:
> We don't seem to actually read / use the profile in this case :)  Any reason?
> 
The prupose is to track the prevalence of ICC v4 profiles in the wild in order
to justify implementing (or not implementing?) ICC v4 support.

> Perhaps we should just record the version of the display profile in another
> stat, using qcms_profile_get_version(m_outputDeviceProfile) and record that if
> needed?

Good idea. I will create a separate histogram just for profile versions

[Noel Gordon, 2015-12-23 16:01:11]

On 2015/12/23 15:58:10, Justin Novosad wrote:
> Thanks for the detailed feedback.  Will look into this in the new year.

Thanks Justin, and I expected you might be headed off soon :)  Wishing you and
yours the seasons greetings, and looking forward to seeing you in the new year.
~noel

[Noel Gordon, 2016-01-20 18:19:37]

Welcome back: any updates here :)

[Justin Novosad, 2016-01-21 21:51:56]

On 2016/01/20 18:19:37, noel gordon wrote:
> Welcome back: any updates here :)

I am swamped if you want to take over this patch, be my guest. Otherwise, I will
likely get back to it around the end of February.

[rkaplow, 2017-02-24 16:04:21]

rkaplow@chromium.org changed reviewers:
- rkaplow@chromium.org