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

Issue 823547 link

Starred by 9 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Use SD slider in RS4

Project Member Reported by hubbe@chromium.org, Mar 20 2018

Issue description

Chrome Version: ToT
OS: Windows

Chrome should use the SD boost slider to figure out brightness on new versions of windows.

 

Comment 1 by wfh@chromium.org, Apr 28 2018

Blockedon: 837888

Comment 2 by hubbe@chromium.org, Apr 30 2018

Blockedon: -837888
This is not dependent on knowing the windows version.
It may be dependent on an SDK update though, not sure yet.

Project Member

Comment 3 by bugdroid1@chromium.org, May 8 2018

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

commit 1017b485eea403c266e055db97941e06fa1a73c3
Author: Fredrik Hubinette <hubbe@google.com>
Date: Tue May 08 03:39:04 2018

Make scalable color spaces.

This is the first step towards implemeing support for
the "SDR brightness" slider in windows 10 RS4. It may also
come in handy for implementing brightness-adjusted
pseudo-HDR on android devices.

Bug:  823547 
Change-Id: If9dbdb1b0c56916d3422ba502c3322d9cfe9b385
Reviewed-on: https://chromium-review.googlesource.com/1048685
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Fredrik Hubinette <hubbe@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556678}
[modify] https://crrev.com/1017b485eea403c266e055db97941e06fa1a73c3/ui/gfx/color_space.cc
[modify] https://crrev.com/1017b485eea403c266e055db97941e06fa1a73c3/ui/gfx/color_space.h
[modify] https://crrev.com/1017b485eea403c266e055db97941e06fa1a73c3/ui/gfx/color_transform_unittest.cc

 Issue 843029  has been merged into this issue.

Comment 5 by hubbe@chromium.org, May 17 2018

Blockedon: 834213

Comment 6 by hubbe@chromium.org, May 18 2018

Status: Fixed (was: Assigned)

Comment 7 by thakis@chromium.org, Jun 11 2018

hubbe: I think you might've picked the wrong blocker in comment 5; that bug looks unrelated to me.

Comment 8 by hubbe@chromium.org, Jun 11 2018

Blockedon: -834213
I think you're right.

Hubbe@ - How can I tell when this makes it to Canary?  

How about merge to beta?  It's not a big crowd, but there are maybe 30 end users watching the Forum HDR threads for a fix

Comment 10 by hubbe@chromium.org, Jun 11 2018

This should already be in canary and beta.
(It's included in 68.0.3425.0 or later.)

hubbe@ - Thanks, although I don't see how it's been incorporated in any public builds.
68.0.3425.0 went to Canary around 5/10.
This  bug 823547  was blocked (on 834213) until today 6/10.

I've posted canary & beta availability notices to 2 Chrome forum HDR topics.
Stay tuned for feedback..

Comment 12 by hubbe@chromium.org, Jun 11 2018

The blocking was a mistake and has no actual effect on the change that was submitted.

hubbe@ Does blocking in general not inhibit incorporation, 
or was in only this block which was to a master compiler CR
  that itself was green in canary ?

Your description makes blocked-on sound like a cosmetic, editorial flag, without (direct) build impact.

Thanks, again.  I have a CM background and had always just assumed blocking was a build distribution operative.

Comment 14 by hubbe@chromium.org, Jun 12 2018

Blocking only affects bug tracking.

Unfortunately I messed up and did link the CL that actually implements to this bug, but you can see it here: https://chromium-review.googlesource.com/c/chromium/src/+/1054704

The version number in #10 was also incorrect, because I looked at the wrong code. The version that actually includes the above change is: 68.0.3435.0

The change relies on an updated SDK, and will not (cannot) be merged into earlier versions of chrome unfortunately.

Since Win canary is currently at 69.* and beta is at 68.0.3440.17 that still means that the code that reads the SD slider should be active on both canary and beta channels.

hubbe@ Thanks for updating the canary build to 68.0.3435 ~5/19.

I scanned the 2 Chrome Forum Help threads and found no canary refs (at all) near or after this date - which would have been a sanity/confirmation check, but didn't find anything.

I see the SDK dependency preventing merging this forward to M67. LGTM

I'll forward and beta user feedback I see

Comment 16 Deleted

The confirmation reported 6/13 was wrong - OK in stable 67.0.3396.87 - NA
(deleted)

3 'not fixed' in beta reports in the Chrome forum - where the display is gray: See merged  issue 843029 .

Still waiting for 1803 user confirmation.
The Chrome forum thread is here:
 https://productforums.google.com/forum/#!topic/chrome/lAiOdlHS-XM 

Comment 19 by hubbe@chromium.org, Jun 14 2018

Tested this on my machine (which is a Win 10 machine with a nvidia GPU) and both chrome canary and chrome beta behaved as expected. So I guess the question is what the difference is between my machine and the machines where this doesn't seem to work well.  For instance, maybe this is a multi-screen issue? (My computer only has one screen, so it's difficult for me to test this theory.)

Maybe it's the SDR value?

tsujimasen said:
Beta is Brightness matched with other SDR windows at 33, but when SDR Brightness is set to 100 like I prefer, Chrome looks grey. When set to 0, other SDR content is grey while Chrome is white.

André Valgrande said:
...If I enable HDR on Windows, set the SDR content brightness to something bright, then everything looks good... but Chrome. HDR videos on YouTube will display properly, menus on Chrome will display properly (whites are white), but the regular windows and content are just dim. Every other app on my computer displays properly.

==
I skimmed the thread for this scorecard summary:

Andre: beta still bad, older cpu: 4th gen i5
Graciel: beta still bad, gtx 170
Angelo: TBD: chrome version?, vega56
Josh: beta still bad, i7 7700k, R9 380 GPU

hubbe: If you have remarks for the users, please post directly to the thread: 
  

Comment 21 by hubbe@chromium.org, Jun 15 2018

It would seem that this is not really fixed.
Chrome appears to be using a fixed value rather than the value of the slider.
Will investigate further.

Project Member

Comment 22 by bugdroid1@chromium.org, Jun 15 2018

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

commit 5cdf4932755b820d08e9f1589a073b86273ea013
Author: Fredrik Hubinette <hubbe@google.com>
Date: Fri Jun 15 23:59:03 2018

Fix SD slider reading in HD

A simple typo prevented this from working.
My manual testing didn't notice because the default value
matches my desktop.

Bug:  823547 
Change-Id: I9a497380c17a85437b8486e0160f6df7a1429922
Reviewed-on: https://chromium-review.googlesource.com/1103231
Commit-Queue: Fredrik Hubinette <hubbe@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567831}
[modify] https://crrev.com/5cdf4932755b820d08e9f1589a073b86273ea013/ui/display/win/screen_win.cc

Comment 23 by hubbe@chromium.org, Jun 16 2018

Labels: Merge-Request-68

Comment 24 by hubbe@chromium.org, Jun 16 2018

Labels: -Pri-3 Pri-2

Comment 25 by hubbe@chromium.org, Jun 16 2018

To be clear: Only the last CL needs to be merged, and it's a two-line change.

Outstanding! Will it be in the next canary? After today's 69.0.3461.2?
Project Member

Comment 27 by sheriffbot@chromium.org, Jun 17 2018

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
User confirmed OK in canary 69.0.3462.0 here
https://productforums.google.com/d/msg/chrome/lAiOdlHS-XM/qnpFgD3wBQAJ

Hope this helps the beta merge signoff
Labels: -Merge-Review-68 Merge-Approved-68
Approved - branch:3440
Cc: abdulsyed@chromium.org
Pls merge you change to M68 branch 3440 ASAP so we can pick it up for this week Beta release. Merge has to happen latest by 1:00 PM PT tomorrow, Tuesday (06/19), so we can pick it up for Wednesday Beta release.




Comment 31 by hubbe@chromium.org, Jun 19 2018

Labels: Merge-Merged
Merged as 0bda1eac5b950ad425e4ad5c98dda0a28c4bfc1e (from gerrit)
Not sure why the status wasn't updated automatically.

Project Member

Comment 32 by bugdroid1@chromium.org, Jun 20 2018

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37e839bcca13d1769a83696cb68de0f55f12d33b

commit 37e839bcca13d1769a83696cb68de0f55f12d33b
Author: Fredrik Hubinette <hubbe@google.com>
Date: Wed Jun 20 21:09:38 2018

Fix SD slider reading in HD

A simple typo prevented this from working.
My manual testing didn't notice because the default value
matches my desktop.

Bug:  823547 
Change-Id: I9a497380c17a85437b8486e0160f6df7a1429922
Reviewed-on: https://chromium-review.googlesource.com/1103231
Commit-Queue: Fredrik Hubinette <hubbe@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#567831}(cherry picked from commit 5cdf4932755b820d08e9f1589a073b86273ea013)
Reviewed-on: https://chromium-review.googlesource.com/1105137
Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#467}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/37e839bcca13d1769a83696cb68de0f55f12d33b/ui/display/win/screen_win.cc

Hubbe@ - Is there a preferred follow on CR for dimmed/overexposed?  I'm still seeing 'overexposed' failures in the forum:
  https://productforums.google.com/forum/#!topic/chrome/lAiOdlHS-XM

Sign in to add a comment