New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 771345 link

Starred by 8 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 20
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-03-19
OS: Chrome
Pri: 3
Type: Bug

Blocked on:
issue 779311
issue 809709
issue 809909
issue 829506


Participants' hotlists:
high-bit-depth


Sign in to add a comment

Retrieve and use monitor's color space characteristics

Project Member Reported by mcasas@chromium.org, Oct 3 2017

Issue description

CrOs does not retrieve nor use the color space characteristics of the 
monitor(s), which makes the rendering of wide color gamut images
such as e.g. those in [1] fail (in cases where monitors support it).

(Note that X11

[1] https://webkit.org/blog-files/color-gamut/
 
Note that X11 retrieves the ICC profile of the monitor via querying the
appropriate X property [1], which is populated by colord via some magic
monitor recognition. We don't have such backend in CrOs now.

[1] https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_screen_x11.cc?type=cs&l=380

Comment 2 by hubbe@chromium.org, Oct 3 2017

In theory, it's possible to read some information about the monitor from it's EDID reply, but I've been informed that doing so is not usually a good idea since the returned EDID data is not very reliable.

We seem to be getting something useful out of EDID on the boards (eve, soraka and I suspect kevin is the same as eve) we tested. Parsing color information from EDID is probably a good starting point.
True, they sometimes don't contain the primaries and/or are inaccurate,
so we'll take them with a grain of salt. Some results:

HP z32x   red x   red y  green x green y  blue x  blue y  white x white y
         0.6738, 0.3164,  0.1962, 0.7197, 0.1484, 0.0439, 0.3144, 0.3291
AdobeRGB 0.64    0.33     0.21    0.71    0.15    0.06    0.3127  0.3290

Samus    0.6337, 0.3476,  0.3212, 0.5771, 0.1513, 0.0908, 0.3144, 0.3291
BT709    0.64    0.33     0.30    0.60    0.15    0.06    0.3127  0.3290

HP z32x claims to be Adobe RGB and Samus is a BT.709.

Comment 5 by hubbe@chromium.org, Oct 3 2017

Do we have a way for users to install their own ICC/ICM profiles for a monitor in CRoS?

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb7e2441dd8878f733e43799ea77c2bab66816d3

commit eb7e2441dd8878f733e43799ea77c2bab66816d3
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Thu Oct 05 22:07:34 2017

Add service function to parse color information from monitor EDID

This CL adds a ParseChromaticityCoordinates() service function to the
bunch of edid_parser.cc functions, and adds unittests with a few real
EDIDs collected in the wild.

ParseOutputDeviceData() in that very file is minimally refactored
for early-return.

R=edid_parser_fuzzer

Bug:  771345 
Change-Id: I956609d381b6fb199021b725f3838f55fb1fda48
Reviewed-on: https://chromium-review.googlesource.com/699351
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506876}
[modify] https://crrev.com/eb7e2441dd8878f733e43799ea77c2bab66816d3/ui/display/BUILD.gn
[modify] https://crrev.com/eb7e2441dd8878f733e43799ea77c2bab66816d3/ui/display/util/BUILD.gn
[add] https://crrev.com/eb7e2441dd8878f733e43799ea77c2bab66816d3/ui/display/util/DEPS
[modify] https://crrev.com/eb7e2441dd8878f733e43799ea77c2bab66816d3/ui/display/util/edid_parser.cc
[modify] https://crrev.com/eb7e2441dd8878f733e43799ea77c2bab66816d3/ui/display/util/edid_parser.h
[modify] https://crrev.com/eb7e2441dd8878f733e43799ea77c2bab66816d3/ui/display/util/edid_parser_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/231a3f03dae135ecdeb285804774ea9fd53f86e0

commit 231a3f03dae135ecdeb285804774ea9fd53f86e0
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Fri Oct 06 20:38:58 2017

Cleanup data types in edid_parser helper functions

This CL corrects the use of "unsigned int/char" etc to use instead
size_t and uint8_t, which is the correct way to calculate offsets
in arrays, and to address byte values.

No new code is intended.

See, e.g.
https://www.securecoding.cert.org/confluence/display/c/INT01-C.+Use+rsize_t+or+size_t+for+all+integer+values+representing+the+size+of+an+object

and
https://www.securecoding.cert.org/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands

for more context.



Bug:  771345 
Change-Id: I6d179afd626b98a4a42f374ae1ce14685fa1f076
Reviewed-on: https://chromium-review.googlesource.com/705114
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507168}
[modify] https://crrev.com/231a3f03dae135ecdeb285804774ea9fd53f86e0/ui/display/util/edid_parser.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01fb329b47b1f377044f0c73751c41a745398fe2

commit 01fb329b47b1f377044f0c73751c41a745398fe2
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Mon Oct 09 21:32:22 2017

Add function to parse gamma and bit depth from EDID monitor information

This CL adds two service functions to edid_parser.cc: one for
extracting the gamma value and another for extracting the monitor's
bit-per-color-channel depth in bits.

Unittests updated.

Bug:  771345 
Change-Id: If860ce1af48c85e3d8e70a4d0c006f90b51dd3d4
Reviewed-on: https://chromium-review.googlesource.com/704169
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507495}
[modify] https://crrev.com/01fb329b47b1f377044f0c73751c41a745398fe2/ui/display/util/edid_parser.cc
[modify] https://crrev.com/01fb329b47b1f377044f0c73751c41a745398fe2/ui/display/util/edid_parser.h
[modify] https://crrev.com/01fb329b47b1f377044f0c73751c41a745398fe2/ui/display/util/edid_parser_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c08fc3300a7dbc92472c16dcc2cd8983a3261e54

commit c08fc3300a7dbc92472c16dcc2cd8983a3261e54
Author: Daniele Castagna <dcastagna@chromium.org>
Date: Tue Oct 10 17:19:09 2017

ash: Add UMA stats to DisplayColorManager.

Add three UMA stats to DisplayColorManager:
- Whether a display has "CTM" property.
- Whether we could parse a valid product ID.
- If we could find an ICC file given a valid product ID.

The goal of these UMA stats is to investigate the effectiveness
of the ICC service ChromeOS currently use for color management.

Bug:  771345 
Change-Id: Ib78f7c0c1661393dd5a93f99147a12aeeaa55281
Reviewed-on: https://chromium-review.googlesource.com/706664
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507702}
[modify] https://crrev.com/c08fc3300a7dbc92472c16dcc2cd8983a3261e54/ash/display/display_color_manager_chromeos.cc
[modify] https://crrev.com/c08fc3300a7dbc92472c16dcc2cd8983a3261e54/tools/metrics/histograms/histograms.xml

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a70106312e039581f2636dd2fd3f8b7799dced34

commit a70106312e039581f2636dd2fd3f8b7799dced34
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Tue Oct 10 20:56:14 2017

Add display::Display info to chrome://gpu

This CL adds a new section to chrome://gpu and dumps the Display(s)
info on it, concretely the ToString() and the color space related
info. It should look like [1,2]

[1] https://i.imgur.com/Z8EPito.png (or https://imgur.com/a/NkpeK)
[2] https://i.imgur.com/xvi3iwj.jpg (https://imgur.com/a/2flBM)

Bug:  771345 
Change-Id: I4fa891a09f2a6424d98b839020c30681c67acdc6
Reviewed-on: https://chromium-review.googlesource.com/709615
Reviewed-by: ccameron chromium <ccameron@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507766}
[modify] https://crrev.com/a70106312e039581f2636dd2fd3f8b7799dced34/content/browser/gpu/gpu_internals_ui.cc
[modify] https://crrev.com/a70106312e039581f2636dd2fd3f8b7799dced34/content/browser/resources/gpu/info_view.html
[modify] https://crrev.com/a70106312e039581f2636dd2fd3f8b7799dced34/content/browser/resources/gpu/info_view.js
[modify] https://crrev.com/a70106312e039581f2636dd2fd3f8b7799dced34/ui/gfx/color_space.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92f328e4ab805bcefc5d5a5b1e2fc975886b06e8

commit 92f328e4ab805bcefc5d5a5b1e2fc975886b06e8
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Tue Oct 10 22:54:31 2017

Move cc::MathUtil::ApproximatelyEqual() from cc/base/math_util.h to base/numerics/ranges.h

This CL moves the utility function cc::MathUtil::ApproximatelyEqual() from
//cc/base/math_util.h to //base/numerics/ranges.h, so that it can be used
from folders that don't need/ can't to depend on //cc.

It also adds an static_assert for std::is_arithmetic<>, and removes the
DCHECK internal (because we don't depend on base/logging.h inside
base/numerics).

Bug:  771345 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I2617abe630774c3148df1507afcb57257ed394bc
Reviewed-on: https://chromium-review.googlesource.com/704208
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507807}
[modify] https://crrev.com/92f328e4ab805bcefc5d5a5b1e2fc975886b06e8/base/numerics/ranges.h
[modify] https://crrev.com/92f328e4ab805bcefc5d5a5b1e2fc975886b06e8/cc/animation/transform_operation.cc
[modify] https://crrev.com/92f328e4ab805bcefc5d5a5b1e2fc975886b06e8/cc/base/math_util.h
[modify] https://crrev.com/92f328e4ab805bcefc5d5a5b1e2fc975886b06e8/chrome/browser/vr/animation_player.cc
[modify] https://crrev.com/92f328e4ab805bcefc5d5a5b1e2fc975886b06e8/chrome/browser/vr/elements/viewport_aware_root_unittest.cc
[modify] https://crrev.com/92f328e4ab805bcefc5d5a5b1e2fc975886b06e8/chrome/browser/vr/ui_scene_manager_unittest.cc
[modify] https://crrev.com/92f328e4ab805bcefc5d5a5b1e2fc975886b06e8/ui/display/util/edid_parser_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1079322015fd7d5109e83b2c8ff2e1132635066b

commit 1079322015fd7d5109e83b2c8ff2e1132635066b
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Wed Oct 11 17:45:44 2017

ozone/drm: (cleanup) use DisplaySnapshot for Display info collection and delivery

DrmDisplay composes a DisplaySnapshot_Params struct in GPU process
that is sent over classic IPC to the Host side, where it is converted
to a DisplaySnapshot class.

This CL replaces the uses of DisplaySnapshot_Params in GPU code
with DisplaySnapshot; this was bound to happen anyway with a move
to Mojo (see comment [1]) and Mojo knows how to serialize
DisplaySnapshot [2], so the _Params struct would be nuked.

Added bonus is that DisplaySnapshot is move-only, so the whole passing
around data gets clearer.

No new code nor functionality is intended.

[1] https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/common/drm_util.cc?q=CreateMovableDisplaySnapshotsFromParams&dr=CSs&l=400
[2] https://cs.chromium.org/chromium/src/ui/display/mojo/display_snapshot_struct_traits.h?dr=CSs

Bug:  771345 
Change-Id: Icc2dba23fd2f9c2a00e99e0fa4c74aa4950b8655
Reviewed-on: https://chromium-review.googlesource.com/706474
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508024}
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/common/drm_util.cc
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/common/drm_util.h
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/common/drm_util_unittest.cc
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/gpu/drm_display.cc
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/gpu/drm_display.h
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/gpu/drm_gpu_display_manager.h
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/gpu/drm_thread.cc
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/gpu/drm_thread_message_proxy.cc
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/host/drm_display_host.cc
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/host/drm_display_host.h
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/host/drm_display_host_manager.cc
[modify] https://crrev.com/1079322015fd7d5109e83b2c8ff2e1132635066b/ui/ozone/platform/drm/host/host_drm_device.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed92f7700ef08d6c2fc8f9fc18af538d9977a449

commit ed92f7700ef08d6c2fc8f9fc18af538d9977a449
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Wed Oct 11 20:51:57 2017

ui/display: cleanup DisplayMode and DisplaySnapshot

This CL cleans up a bit the said classes:
- removes unnecessary virtuals in DisplayMode and DisplaySnapshot
 (there are no classes deriving the former and
 only FakeDisplaySnapshot derives the latter).
- makes members const where appropriate
- reorders accessors/mutators of DisplaySnapshot to
 reflect the members order.
- some minor style update in display_mode.cc

No new code is intended, just brush up the existing.

Bug:  771345 
Change-Id: If34b8ae822d876ed0b4397129fe6ffd9c72b276d
Reviewed-on: https://chromium-review.googlesource.com/713175
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508106}
[modify] https://crrev.com/ed92f7700ef08d6c2fc8f9fc18af538d9977a449/ui/display/types/display_mode.cc
[modify] https://crrev.com/ed92f7700ef08d6c2fc8f9fc18af538d9977a449/ui/display/types/display_mode.h
[modify] https://crrev.com/ed92f7700ef08d6c2fc8f9fc18af538d9977a449/ui/display/types/display_snapshot.h

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6eb6db25f842224218ab918476c970912812f007

commit 6eb6db25f842224218ab918476c970912812f007
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Thu Oct 12 02:01:28 2017

ui/display: use constexpr ISO const in edid_parser where applicable

This CL is a follow up to comment [1], substituting const with constexpr
where appropriate in edid_parser.cc following the rec in [2].

No new code is intended.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/704169/4/ui/display/util/edid_parser.cc#345
[2] https://chromium-cpp.appspot.com/

Bug:  771345 
Change-Id: I4e18a177a5465617dbc98661a93d8b1870df8fb4
Reviewed-on: https://chromium-review.googlesource.com/714027
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508220}
[modify] https://crrev.com/6eb6db25f842224218ab918476c970912812f007/ui/display/util/edid_parser.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b13f0514314c5a6b0147566d682734d74b0a5db

commit 7b13f0514314c5a6b0147566d682734d74b0a5db
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Tue Oct 17 16:42:52 2017

ozone/drm: add a gfx::ColorSpace member to DisplaySnapshot

This CL adds a gfx::ColorSpace |color_space_| to DisplaySnapshot
and teaches both IPC and Mojo how to serialize/deserialize it.

drm_util.cc also gets a new method to create a gfx::ColorSpace
out of edid_parser.{cc,h} functions, and uses it to fill in the
said class.

Bug:  771345 
Change-Id: I6b7ea390e730a38b83272d772039d4a06e914af3
Reviewed-on: https://chromium-review.googlesource.com/709696
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509411}
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/manager/fake_display_snapshot.cc
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/mojo/BUILD.gn
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/mojo/display_snapshot.mojom
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/mojo/display_snapshot_struct_traits.cc
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/mojo/display_snapshot_struct_traits.h
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/mojo/display_struct_traits_unittest.cc
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/types/BUILD.gn
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/types/DEPS
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/types/display_snapshot.cc
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/display/types/display_snapshot.h
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/ozone/common/BUILD.gn
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/ozone/common/gpu/ozone_gpu_message_params.h
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/ozone/common/gpu/ozone_gpu_messages.h
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/ozone/platform/drm/common/drm_util.cc
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/ozone/platform/drm/common/drm_util.h
[modify] https://crrev.com/7b13f0514314c5a6b0147566d682734d74b0a5db/ui/ozone/platform/drm/common/drm_util_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/792e1d3b75428118c67a8e59dc38dca9cbe63928

commit 792e1d3b75428118c67a8e59dc38dca9cbe63928
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Thu Oct 19 14:15:40 2017

ui/ozone: add sanity checks to EDID to gfx::ColorSpace conversion

This CL adds sanity checks in the creation of gfx::ColorSpace 
from a monitor's EDID. The checks are: making sure that the 
primaries are correctly located relative to each other, and that
the area covered by the triangle of primaries is not too small.

This CL also adds a wacky EDID to the drm_util_unittest.cc, 
which primaries can be seen painted on a CIE chromaticity 
diagram in [1], and uses it to test the new sanity checks.

The "sanity checks" are implemented at the drm_util level because
technically speaking the EDID parsing succeeds.

[1] https://i.imgur.com/JYk2rVG.png (or https://imgur.com/a/F7Bvo)

Bug:  771345 
Change-Id: Ia7c85f409383f340cc9deb4633d2a9bd8085ccab
Reviewed-on: https://chromium-review.googlesource.com/723164
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510077}
[modify] https://crrev.com/792e1d3b75428118c67a8e59dc38dca9cbe63928/ui/ozone/platform/drm/common/drm_util.cc
[modify] https://crrev.com/792e1d3b75428118c67a8e59dc38dca9cbe63928/ui/ozone/platform/drm/common/drm_util_unittest.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Oct 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be6c66639cb1463471556de4030b419ddc83878d

commit be6c66639cb1463471556de4030b419ddc83878d
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Thu Oct 19 16:40:23 2017

ui/display (cleanup): make ManagedDisplayMode not ref counted

This CL makes ManagedDisplayMode copyable and not ref-counted
-- this ref-counted-ness was probably so to allow for two
find-like methods of DisplayManager to encode in the return value
the found-or-not; this CL changed those two signatures in
DisplayManager to return a boolean and take an argument:

bool GetActiveModeForDisplayId(int64_t display_id,
                               ManagedDisplayMode* mode)
bool GetSelectedModeForDisplayId(int64_t display_id,
                                 ManagedDisplayMode* mode)

The rest is just a lot of updating all call sites.

No new functionality is intended.

Sizeof(ManagedDisplayMode)= 28B with ref count, 24B w/o

Bug:  771345 
Change-Id: I975635db2a0709389a29c3350da32d846a9fe8ef
Reviewed-on: https://chromium-review.googlesource.com/719996
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510109}
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ash/display/display_manager_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ash/display/resolution_notification_controller.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ash/display/resolution_notification_controller.h
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ash/display/resolution_notification_controller_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ash/display/window_tree_host_manager.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/chrome/browser/chromeos/display/display_preferences.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/chrome/browser/chromeos/display/display_preferences_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/chrome/browser/extensions/display_info_provider_chromeos.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/components/exo/shell_surface_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/components/exo/wm_helper_ash.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/chromeos/display_change_observer.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/chromeos/display_change_observer_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/chromeos/touch_transform_controller_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/chromeos/touchscreen_util_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/display_manager.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/display_manager.h
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/display_manager_utilities.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/display_manager_utilities.h
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/managed_display_info.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/managed_display_info.h
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/manager/managed_display_info_unittest.cc
[modify] https://crrev.com/be6c66639cb1463471556de4030b419ddc83878d/ui/display/test/display_manager_test_api.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Oct 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/554025961861c072de6ceb18392ed1e296a5107a

commit 554025961861c072de6ceb18392ed1e296a5107a
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Tue Oct 24 19:14:12 2017

ui/display: minor cleanup in DisplayManager and ManagedDisplayInfo

This CL cleans up a bit some parts of DisplayManager and
ManagedDisplayInfo, as a splinter of the mentioned bug.

- DisplayManager is a final class, no need for virtual dtor
- DisplayManager changed all {V}LOG into D{V}LOG because 
 "prefer DVLOG(1) to other logging methods" [1]),.
- tried using __func__ instead of the explicit name of the method.
- Used {} in if-else if body or condition > 1 line.
- Reflowed some comments where it made sense (less lines).
- Used early return in 2 methods.
- Removed unnecessary ManagedDisplayInfo::Use125DSFForUIScaling()
 (the comment hinted at it being incorrectly implemented)
- Cleaned up ManagedDisplayMode constructors, using default
member values instead, following the existing |is_default_|.

** No new code is intended **

[1] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#logging


Bug:  771345 
Change-Id: Ib9ecc8d1e5f28e34c5a45e6f597891d17d8b94c4
Reviewed-on: https://chromium-review.googlesource.com/732568
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511228}
[modify] https://crrev.com/554025961861c072de6ceb18392ed1e296a5107a/ui/display/manager/display_manager.cc
[modify] https://crrev.com/554025961861c072de6ceb18392ed1e296a5107a/ui/display/manager/display_manager.h
[modify] https://crrev.com/554025961861c072de6ceb18392ed1e296a5107a/ui/display/manager/managed_display_info.cc
[modify] https://crrev.com/554025961861c072de6ceb18392ed1e296a5107a/ui/display/manager/managed_display_info.h

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/852d42a7f6a4937f68eae9a448b0dbe7fab607db

commit 852d42a7f6a4937f68eae9a448b0dbe7fab607db
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Wed Oct 25 22:37:50 2017

ui/display: wire ColorSpace from DisplaySnapshot to Display

This CL adds a gfx::ColorSpace member to ManagedDisplayInfo
and makes sure is correctly propagated from the CrOs display
observer.

When a DisplaySnapshot update has a correct ColorSpace, we
skip fetching/reusing the ICC file from the Quirks server.
A new UMA is added for the case of correct ColorSpace.

Also Ash.DisplayColorManager.IccFileFound is deprecated in
favour of Ash.DisplayColorManager.IccFileDownloaded; the
former bundled too many booleans inside).


The effect of the ColorSpace wiring can be seen while
browsing to [1] on a monitor supporting wide gamut, e.g. on
an HP z32x (conn via DP).
Before: https://imgur.com/a/XZ8Sw
After: https://imgur.com/a/5WxHZ

[1] https://webkit.org/blog-files/color-gamut/comparison.html

R=rkaplow@ suggestion

Bug:  771345 
Change-Id: I9236bc605db1b2f14c93c0ed8af4a77e67ee8f5a
Reviewed-on: https://chromium-review.googlesource.com/728325
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511621}
[modify] https://crrev.com/852d42a7f6a4937f68eae9a448b0dbe7fab607db/ash/display/display_color_manager_chromeos.cc
[modify] https://crrev.com/852d42a7f6a4937f68eae9a448b0dbe7fab607db/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/852d42a7f6a4937f68eae9a448b0dbe7fab607db/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/852d42a7f6a4937f68eae9a448b0dbe7fab607db/ui/display/manager/chromeos/display_change_observer.cc
[modify] https://crrev.com/852d42a7f6a4937f68eae9a448b0dbe7fab607db/ui/display/manager/display_manager.cc
[modify] https://crrev.com/852d42a7f6a4937f68eae9a448b0dbe7fab607db/ui/display/manager/managed_display_info.cc
[modify] https://crrev.com/852d42a7f6a4937f68eae9a448b0dbe7fab607db/ui/display/manager/managed_display_info.h

Blockedon: 779311
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ba32f975545211716e05cc4ce70113ba3136532

commit 8ba32f975545211716e05cc4ce70113ba3136532
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Tue Nov 07 05:03:34 2017

ui/display: add flag to enable use of monitor's color space info

This CL adds a new flag to control if the monitor's provided color
space info is actually propagated at the DisplayManager level, and
also surfaces this flag in chrome://flags. The flag is disabled by
default and the chrome://flags entry allows for enabling it. This
flag will help debug the possibly related  issue 779311 .

Bug:  771345 ,  779311 
Change-Id: I6df69c5fb9ab9954e9ff9f4fc04fe0616594315c
Reviewed-on: https://chromium-review.googlesource.com/754593
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514398}
[modify] https://crrev.com/8ba32f975545211716e05cc4ce70113ba3136532/chrome/browser/about_flags.cc
[modify] https://crrev.com/8ba32f975545211716e05cc4ce70113ba3136532/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/8ba32f975545211716e05cc4ce70113ba3136532/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/8ba32f975545211716e05cc4ce70113ba3136532/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/8ba32f975545211716e05cc4ce70113ba3136532/ui/display/display_switches.cc
[modify] https://crrev.com/8ba32f975545211716e05cc4ce70113ba3136532/ui/display/display_switches.h
[modify] https://crrev.com/8ba32f975545211716e05cc4ce70113ba3136532/ui/display/manager/display_manager.cc

ccameron@ addressed a similar issue on  crbug.com/769677 , delaying the 
color space conversion to display time (as opposed to raster time),
hence avoiding the delays that caused 779311.  I flipped the flag to
active-by-default and saw no delay, so let's land that and monitor for
regressions.
Project Member

Comment 23 by bugdroid1@chromium.org, Feb 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4420416ee2078ea69f781d41397417e969e82df1

commit 4420416ee2078ea69f781d41397417e969e82df1
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Feb 05 18:09:36 2018

Make use-monitor-color-space enabled by default

This CL changes the flag "use-monitor-color-space" to a
FeatureFlag and makes it enabled by default, since the
slow down issues that justified its landing have been
addressed ( crbug.com/769677#c24 ) -- but just in case let's
leave the flag to allow for on-the-spot testing in case
it regresses again.

Tested on soraka simplechrome w/ external Z32x and
https://webkit.org/blog-files/color-gamut/comparison.html

Bug:  771345 
Change-Id: I1f98e6723a4ca1eed7f7678509cb0f3fef70815f
Reviewed-on: https://chromium-review.googlesource.com/899470
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534422}
[modify] https://crrev.com/4420416ee2078ea69f781d41397417e969e82df1/chrome/browser/about_flags.cc
[modify] https://crrev.com/4420416ee2078ea69f781d41397417e969e82df1/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/4420416ee2078ea69f781d41397417e969e82df1/ui/display/display_switches.cc
[modify] https://crrev.com/4420416ee2078ea69f781d41397417e969e82df1/ui/display/display_switches.h
[modify] https://crrev.com/4420416ee2078ea69f781d41397417e969e82df1/ui/display/manager/display_manager.cc

Labels: M-66
NextAction: 2018-03-01
If nothing bursts in flames, we should remove the flag, so setting
a NextAction date for like a month from now.
Blockedon: 809709
The NextAction date has arrived: 2018-03-01
The NextAction date has arrived: 2018-03-15
NextAction: 2018-03-19
Checked yesterday and should be hitting beta promotion in a couple
of days, kicking the can a bit further down the road.
The NextAction date has arrived: 2018-03-19
Project Member

Comment 31 by bugdroid1@chromium.org, Mar 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5796b828ef726660acece00458d1c46e9f07de96

commit 5796b828ef726660acece00458d1c46e9f07de96
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Mar 19 19:36:03 2018

Remove use-monitor-color-space flag

This CL removes the flag to disable this feature, since it
has been in Beta 66.0.3359.30 without any major problems
so far.

Bug:  771345 
Change-Id: I7283cc05ded90ca3ec24ba227fc80654ad5f3325
Reviewed-on: https://chromium-review.googlesource.com/912016
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544122}
[modify] https://crrev.com/5796b828ef726660acece00458d1c46e9f07de96/chrome/browser/about_flags.cc
[modify] https://crrev.com/5796b828ef726660acece00458d1c46e9f07de96/ui/display/display_switches.cc
[modify] https://crrev.com/5796b828ef726660acece00458d1c46e9f07de96/ui/display/display_switches.h
[modify] https://crrev.com/5796b828ef726660acece00458d1c46e9f07de96/ui/display/manager/display_manager.cc

Project Member

Comment 32 by bugdroid1@chromium.org, Apr 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6315430cb03d261dad86b1be3237b3a062035177

commit 6315430cb03d261dad86b1be3237b3a062035177
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Apr 02 21:53:00 2018

Revert "Remove use-monitor-color-space flag"

This reverts commit 5796b828ef726660acece00458d1c46e9f07de96.

Reason for revert: We might need to leave the flag around
for longer than expected due to field bugs such as  crbug.com/809909 


Original change's description:
> Remove use-monitor-color-space flag
> 
> This CL removes the flag to disable this feature, since it
> has been in Beta 66.0.3359.30 without any major problems
> so far.
> 
> Bug:  771345 
> Change-Id: I7283cc05ded90ca3ec24ba227fc80654ad5f3325
> Reviewed-on: https://chromium-review.googlesource.com/912016
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#544122}

TBR=avi@chromium.org,mcasas@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  771345 
Change-Id: Ic6b32229d9fea3c5e3d47d06dbbb596f854fa647
Reviewed-on: https://chromium-review.googlesource.com/990213
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547532}
[modify] https://crrev.com/6315430cb03d261dad86b1be3237b3a062035177/chrome/browser/about_flags.cc
[modify] https://crrev.com/6315430cb03d261dad86b1be3237b3a062035177/ui/display/display_switches.cc
[modify] https://crrev.com/6315430cb03d261dad86b1be3237b3a062035177/ui/display/display_switches.h
[modify] https://crrev.com/6315430cb03d261dad86b1be3237b3a062035177/ui/display/manager/display_manager.cc

Blockedon: 809909
Project Member

Comment 34 by bugdroid1@chromium.org, Apr 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f8a9599429528169837d0b6c981c0b4a04a5660f

commit f8a9599429528169837d0b6c981c0b4a04a5660f
Author: Miguel Casas <mcasas@chromium.org>
Date: Wed Apr 04 18:39:19 2018

display: ignore EDID-provided colorspace if blue is broken

Some EDID-provided chromaticity coordinates for the blue colour are
too far from the expected ones, causing blue to be rendered as purple
(or cyan, in some cases). This is due to broken primaries, as all the
standard colorspaces' blues are clustered nearby.

This CL adds a sanity check for the blue coordinates (need to be to
the left and below of a given point) and adds a unit test for it. If
the condition doesn't verify, we just ignore this ColorSpace -- which
means we'll treat the monitor as sRGB.

Bug:  809909 ,  771345 
Change-Id: I4555625928f883825a3b315abdaaa379eb26a415
Reviewed-on: https://chromium-review.googlesource.com/993513
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548138}
[modify] https://crrev.com/f8a9599429528169837d0b6c981c0b4a04a5660f/ui/ozone/platform/drm/common/drm_util.cc
[modify] https://crrev.com/f8a9599429528169837d0b6c981c0b4a04a5660f/ui/ozone/platform/drm/common/drm_util_unittest.cc

Blockedon: 829506
Labels: -M-66 M-67
Punting to M67 (reverted on M66, see  issue 809909 )
Status: Fixed (was: Started)
Delivered in M67 and merrily soldiering on !!

Sign in to add a comment