Load ICC file for display color correction based on display product identifier

Load ICC file for display color correction based on display product identifier

This change adds a new member to ui::DisplaySnapshot, 'product_id',
which is populated from the EDID in DrmDisplaySnapshot when it is
created.

DisplaySnapshot::product_id() is then turned into a hexadecimal string
and used to try and load an ICC file from a common path.

BUG=471749
TEST=On a link_freon system blue-vcgt-test-color-profile.icc was copied into
/usr/share/color/icc/30e400000000.icc and the system rebooted and the display
was observed tinted blue.

Committed: https://crrev.com/772b4792fd8c99925431f4639aaf92fcd6a9ce10
Cr-Commit-Position: refs/heads/master@{#329888}

[robert.bradford, 2015-05-06 15:13:25]

robert.bradford@intel.com changed reviewers:
+ glevin@chromium.org

[robert.bradford, 2015-05-06 15:36:16]

robert.bradford@intel.com changed reviewers:
+ dcheng@chromium.org, marcheu@chromium.org, oshima@chromium.org,
spang@chromium.org

[robert.bradford, 2015-05-06 15:36:16]

robert.bradford@intel.com changed required reviewers:
+ dcheng@chromium.org, oshima@chromium.org, spang@chromium.org

[robert.bradford, 2015-05-06 15:36:16]

Hi folks, this CL is next step in the display color correction for ChromeOS
devices. Moving beyond a command line to load from, this change loads from a
common directory based on edid.

spang@ please could you review the changes in ui/ozone

oshima@ please could you review the changes in ash, ui/display and chromeos

dcheng@ please could you do ipc security review

marcheu@ Overall feedback on if this works from a ChromeOS perspective.

glevin@ FYI

Thank you all.

[robert.bradford, 2015-05-06 15:36:19]

Hi folks, this CL is next step in the display color correction for ChromeOS
devices. Moving beyond a command line to load from, this change loads from a
common directory based on edid.

spang@ please could you review the changes in ui/ozone

oshima@ please could you review the changes in ash, ui/display and chromeos

dcheng@ please could you do ipc security review

marcheu@ Overall feedback on if this works from a ChromeOS perspective.

glevin@ FYI

Thank you all.

[dcheng, 2015-05-06 18:27:46]

IPC changes lgtm, but one random question

https://codereview.chromium.org/1129863003/diff/1/ui/display/types/display_sn...
File ui/display/types/display_snapshot.h (right):

https://codereview.chromium.org/1129863003/diff/1/ui/display/types/display_sn...
ui/display/types/display_snapshot.h:59: static const int64_t kInvalidProductID =
-1;
Just curious: why is kInvalidProductID -1, and kDummyDisplayId and
kDummyProductId == 1?

[spang, 2015-05-07 18:04:56]

ui/ozone lgtm

[robert.bradford, 2015-05-12 15:48:59]

Thanks spang@ and dcheng@. Still looking for feedback from marcheu@ and oshima@.

On 2015/05/06 18:27:46, dcheng wrote:
> IPC changes lgtm, but one random question
> 
>
https://codereview.chromium.org/1129863003/diff/1/ui/display/types/display_sn...
> File ui/display/types/display_snapshot.h (right):
> 
>
https://codereview.chromium.org/1129863003/diff/1/ui/display/types/display_sn...
> ui/display/types/display_snapshot.h:59: static const int64_t kInvalidProductID
=
> -1;
> Just curious: why is kInvalidProductID -1, and kDummyDisplayId and
> kDummyProductId == 1?

Great question. kDummyProductId and kDummyDisplayId are only used when creating
a DisplaySnapshot from the command line (used for debugging/testing.) These need
to be valid IDs as the synthesised DisplaySnapshot needs to be valid (they could
equally be 1234 or 0xdecafbad.) kInvalidDisplayID on the other hand is used for
testing valid display throughout the code.

[oshima, 2015-05-14 00:06:12]

https://codereview.chromium.org/1129863003/diff/1/ash/content/display/display...
File ash/content/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1129863003/diff/1/ash/content/display/display...
ash/content/display/display_color_manager_chromeos.cc:14: #include
"base/path_service.h"
remove dup.

https://codereview.chromium.org/1129863003/diff/1/ash/content/display/display...
ash/content/display/display_color_manager_chromeos.cc:95: if
(calibration_map_[state->display_id()]) {
shouldn't we cache the value per product id instead of display_id now?

[robert.bradford, 2015-05-14 17:55:38]

Hi oshima@ thanks for the review, we now cache based on product id. I alsi
improved the way the internal display (from command line) is loaded to make it
fit in with the new way better.

I also fixed some s/uint64_t/int64_t and this is rebased and tested on ToT.

https://codereview.chromium.org/1129863003/diff/1/ash/content/display/display...
File ash/content/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1129863003/diff/1/ash/content/display/display...
ash/content/display/display_color_manager_chromeos.cc:14: #include
"base/path_service.h"
On 2015/05/14 00:06:11, oshima wrote:
> remove dup.

Done.

https://codereview.chromium.org/1129863003/diff/1/ash/content/display/display...
ash/content/display/display_color_manager_chromeos.cc:95: if
(calibration_map_[state->display_id()]) {
On 2015/05/14 00:06:11, oshima wrote:
> shouldn't we cache the value per product id instead of display_id now?

Done. But this resulted in a refactor of the way the command line works which I
think is actually better.

[robert.bradford, 2015-05-14 17:55:54]

The CQ bit was checked by robert.bradford@intel.com to run a CQ dry run

[robert.bradford, 2015-05-14 17:55:55]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1129863003/#ps20001
(title: "Address oshima@ feedback and rebase")

[commit-bot: I haz the power, 2015-05-14 17:57:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129863003/20001

[oshima, 2015-05-14 18:11:44]

yep, looks better. lgtm with one nit

https://codereview.chromium.org/1129863003/diff/20001/ash/content/display/dis...
File ash/content/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1129863003/diff/20001/ash/content/display/dis...
ash/content/display/display_color_manager_chromeos.cc:109: }
nit: nuke {}

[robert.bradford, 2015-05-14 18:16:42]

The CQ bit was checked by robert.bradford@intel.com

[robert.bradford, 2015-05-14 18:16:43]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
dcheng@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1129863003/#ps40001
(title: "Nuked extra {}")

[robert.bradford, 2015-05-14 18:17:50]

Thanks oshima!

https://codereview.chromium.org/1129863003/diff/20001/ash/content/display/dis...
File ash/content/display/display_color_manager_chromeos.cc (right):

https://codereview.chromium.org/1129863003/diff/20001/ash/content/display/dis...
ash/content/display/display_color_manager_chromeos.cc:109: }
On 2015/05/14 18:11:43, oshima wrote:
> nit: nuke {}

Done.

[commit-bot: I haz the power, 2015-05-14 18:17:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129863003/40001

[commit-bot: I haz the power, 2015-05-14 18:58:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-14 18:59:29]

Patchset 3 (id:??) landed as
https://crrev.com/772b4792fd8c99925431f4639aaf92fcd6a9ce10
Cr-Commit-Position: refs/heads/master@{#329888}