Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2938543002:
Detect HDR capability (Closed)
DescriptionDetect HDR capability
This calls the windows API for detecting if the output display can do HDR output or not.
Note that this API depends on a recent version of the WinSDK, but has been guarded by ifdefs to make sure that it compiles even without the new SDK. (But without HDR detection.)
BUG=682416
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2938543002
Cr-Commit-Position: refs/heads/master@{#482479}
Committed: https://chromium.googlesource.com/chromium/src/+/b9ab2095d1f334506aa74681fa758adf03ab4a13
Patch Set 1 #Patch Set 2 : didn't mean to include toolchain update #Patch Set 3 : compile fix #
Total comments: 4
Patch Set 4 : merged #Patch Set 5 : moved to dc surface #
Total comments: 15
Patch Set 6 : updated histograms #
Total comments: 6
Patch Set 7 : comments addressed #Patch Set 8 : updated histograms and enums #Patch Set 9 : histograms updated #
Messages
Total messages: 77 (44 generated)
Description was changed from ========== Detect HDR capability This calls the windows API for detecting if the output display can do HDR output or not. Note that this APi depends on a recent version of the WinSDK, but has been guarded by BUG=682416 ========== to ========== Detect HDR capability This calls the windows API for detecting if the output display can do HDR output or not. Note that this APi depends on a recent version of the WinSDK, but has been guarded by BUG=682416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Detect HDR capability This calls the windows API for detecting if the output display can do HDR output or not. Note that this APi depends on a recent version of the WinSDK, but has been guarded by BUG=682416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Detect HDR capability This calls the windows API for detecting if the output display can do HDR output or not. Note that this APi depends on a recent version of the WinSDK, but has been guarded by BUG=682416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
hubbe@chromium.org changed reviewers: + jbauman@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Not sure if this got sent before or not. PTAL.
https://codereview.chromium.org/2938543002/diff/40001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/2938543002/diff/40001/gpu/config/gpu_info.h#n... gpu/config/gpu_info.h:227: bool hdr = false; Add this to MergeGPUInfoGL, GPUInfo::EnumerateFields, GpuInfoAsDictionaryValue(), gpu/ipc/common/gpu_info.mojom, gpu/ipc/common/gpu_param_traits_macros.h, and gpu/ipc/common/gpu_info_struct_traits.h https://codereview.chromium.org/2938543002/diff/40001/gpu/ipc/service/child_w... File gpu/ipc/service/child_window_surface_win.h (right): https://codereview.chromium.org/2938543002/diff/40001/gpu/ipc/service/child_w... gpu/ipc/service/child_window_surface_win.h:23: static bool IsHDRSupported(); Could you add this to DirectCompositionSurfaceWin? I'm trying to get rid of ChildWindowSurfaceWin.
On 2017/06/15 21:23:49, hubbe wrote: > Not sure if this got sent before or not. > PTAL. CL description needs editing - there's a sentence that ends with guarded by I'll see about landing the switch to the latest SDK.
On 2017/06/15 21:48:50, brucedawson wrote: > On 2017/06/15 21:23:49, hubbe wrote: > > Not sure if this got sent before or not. > > PTAL. > > CL description needs editing - there's a sentence that ends with guarded by > > I'll see about landing the switch to the latest SDK. I adjusted crrev.com/2914643003 so that it doesn't force those building with a locally installed toolchain to use the 15063 SDK. This should save non-Google developers from build problems caused by the broken 15063 SDK's wrl\event.h. It means that this code will only be compiled when DEPOT_TOOLS_WIN_TOOLCHAIN=1, which is fine.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_optional_gpu_tests_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2938543002/diff/40001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/2938543002/diff/40001/gpu/config/gpu_info.h#n... gpu/config/gpu_info.h:227: bool hdr = false; On 2017/06/15 21:44:59, jbauman wrote: > Add this to MergeGPUInfoGL, GPUInfo::EnumerateFields, > GpuInfoAsDictionaryValue(), gpu/ipc/common/gpu_info.mojom, > gpu/ipc/common/gpu_param_traits_macros.h, and > gpu/ipc/common/gpu_info_struct_traits.h Done. https://codereview.chromium.org/2938543002/diff/40001/gpu/ipc/service/child_w... File gpu/ipc/service/child_window_surface_win.h (right): https://codereview.chromium.org/2938543002/diff/40001/gpu/ipc/service/child_w... gpu/ipc/service/child_window_surface_win.h:23: static bool IsHDRSupported(); On 2017/06/15 21:45:00, jbauman wrote: > Could you add this to DirectCompositionSurfaceWin? I'm trying to get rid of > ChildWindowSurfaceWin. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/20 23:09:34, hubbe wrote: > https://codereview.chromium.org/2938543002/diff/40001/gpu/config/gpu_info.h > File gpu/config/gpu_info.h (right): > > https://codereview.chromium.org/2938543002/diff/40001/gpu/config/gpu_info.h#n... > gpu/config/gpu_info.h:227: bool hdr = false; > On 2017/06/15 21:44:59, jbauman wrote: > > Add this to MergeGPUInfoGL, GPUInfo::EnumerateFields, > > GpuInfoAsDictionaryValue(), gpu/ipc/common/gpu_info.mojom, > > gpu/ipc/common/gpu_param_traits_macros.h, and > > gpu/ipc/common/gpu_info_struct_traits.h > > Done. > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ipc/service/child_w... > File gpu/ipc/service/child_window_surface_win.h (right): > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ipc/service/child_w... > gpu/ipc/service/child_window_surface_win.h:23: static bool IsHDRSupported(); > On 2017/06/15 21:45:00, jbauman wrote: > > Could you add this to DirectCompositionSurfaceWin? I'm trying to get rid of > > ChildWindowSurfaceWin. > > Done. Ping?
Hmm, I'm trying to figure out where the right place to make the "hdr mode" decision. Also, I'm trying to figure out where to store the bool that is the result of that decision. I suppose I could just add the kEnableHDR flag to the command line if we decide to enable HDR mode, but that seems really hacky. Any ideas? On Thu, Jun 22, 2017 at 2:54 PM, <hubbe@chromium.org> wrote: > On 2017/06/20 23:09:34, hubbe wrote: > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ > config/gpu_info.h > > File gpu/config/gpu_info.h (right): > > > > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ > config/gpu_info.h#newcode227 > > gpu/config/gpu_info.h:227: bool hdr = false; > > On 2017/06/15 21:44:59, jbauman wrote: > > > Add this to MergeGPUInfoGL, GPUInfo::EnumerateFields, > > > GpuInfoAsDictionaryValue(), gpu/ipc/common/gpu_info.mojom, > > > gpu/ipc/common/gpu_param_traits_macros.h, and > > > gpu/ipc/common/gpu_info_struct_traits.h > > > > Done. > > > > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ > ipc/service/child_window_surface_win.h > > File gpu/ipc/service/child_window_surface_win.h (right): > > > > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ > ipc/service/child_window_surface_win.h#newcode23 > > gpu/ipc/service/child_window_surface_win.h:23: static bool > IsHDRSupported(); > > On 2017/06/15 21:45:00, jbauman wrote: > > > Could you add this to DirectCompositionSurfaceWin? I'm trying to get > rid of > > > ChildWindowSurfaceWin. > > > > Done. > > Ping? > > > https://codereview.chromium.org/2938543002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
On 2017/06/22 23:37:42, chromium-reviews wrote: > Hmm, I'm trying to figure out where the right place to make the "hdr mode" > decision. > Also, I'm trying to figure out where to store the bool that is the result > of that decision. > I suppose I could just add the kEnableHDR flag to the command line if we > decide to enable HDR mode, but that seems really hacky. > Any ideas? > > One difficulty is that the GPU process starts after the first browser compositor and renderer are created. Is there a way to switch HDR modes while the compositor is running? If so we could plumb it in from the GpuInfo, which is sent to every process on GPU channel creation. > On Thu, Jun 22, 2017 at 2:54 PM, <mailto:hubbe@chromium.org> wrote: > > > On 2017/06/20 23:09:34, hubbe wrote: > > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ > > config/gpu_info.h > > > File gpu/config/gpu_info.h (right): > > > > > > > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ > > config/gpu_info.h#newcode227 > > > gpu/config/gpu_info.h:227: bool hdr = false; > > > On 2017/06/15 21:44:59, jbauman wrote: > > > > Add this to MergeGPUInfoGL, GPUInfo::EnumerateFields, > > > > GpuInfoAsDictionaryValue(), gpu/ipc/common/gpu_info.mojom, > > > > gpu/ipc/common/gpu_param_traits_macros.h, and > > > > gpu/ipc/common/gpu_info_struct_traits.h > > > > > > Done. > > > > > > > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ > > ipc/service/child_window_surface_win.h > > > File gpu/ipc/service/child_window_surface_win.h (right): > > > > > > > > https://codereview.chromium.org/2938543002/diff/40001/gpu/ > > ipc/service/child_window_surface_win.h#newcode23 > > > gpu/ipc/service/child_window_surface_win.h:23: static bool > > IsHDRSupported(); > > > On 2017/06/15 21:45:00, jbauman wrote: > > > > Could you add this to DirectCompositionSurfaceWin? I'm trying to get > > rid of > > > > ChildWindowSurfaceWin. > > > > > > Done. > > > > Ping? > > > > > > https://codereview.chromium.org/2938543002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2017/06/23 02:47:17, jbauman wrote: > On 2017/06/22 23:37:42, chromium-reviews wrote: > > Hmm, I'm trying to figure out where the right place to make the "hdr mode" > > decision. > > Also, I'm trying to figure out where to store the bool that is the result > > of that decision. > > I suppose I could just add the kEnableHDR flag to the command line if we > > decide to enable HDR mode, but that seems really hacky. > > Any ideas? > > > > > One difficulty is that the GPU process starts after the first browser compositor > and renderer are created. Is there a way to switch HDR modes while the > compositor is running? If so we could plumb it in from the GpuInfo, which is > sent to every process on GPU channel creation. > So maybe we should be doing this in the browser process (somewhere) ?
On 2017/06/23 03:46:09, hubbe wrote: > On 2017/06/23 02:47:17, jbauman wrote: > > On 2017/06/22 23:37:42, chromium-reviews wrote: > > > Hmm, I'm trying to figure out where the right place to make the "hdr mode" > > > decision. > > > Also, I'm trying to figure out where to store the bool that is the result > > > of that decision. > > > I suppose I could just add the kEnableHDR flag to the command line if we > > > decide to enable HDR mode, but that seems really hacky. > > > Any ideas? > > > > > > > > One difficulty is that the GPU process starts after the first browser > compositor > > and renderer are created. Is there a way to switch HDR modes while the > > compositor is running? If so we could plumb it in from the GpuInfo, which is > > sent to every process on GPU channel creation. > > > > So maybe we should be doing this in the browser process (somewhere) ? I'm not sure - we'd prefer not to create a D3D11 device in the browser process, as that hurts startup time (25ms maybe) and makes it more likely to crash with broken drivers.
The CQ bit was checked by hubbe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hubbe@chromium.org changed reviewers: + palmer@chromium.org
+palmer for security review
Your CL description looks like it got truncated. :) Also "APi" -> "API".
Description was changed from ========== Detect HDR capability This calls the windows API for detecting if the output display can do HDR output or not. Note that this APi depends on a recent version of the WinSDK, but has been guarded by BUG=682416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Detect HDR capability This calls the windows API for detecting if the output display can do HDR output or not. Note that this API depends on a recent version of the WinSDK, but has been guarded by ifdefs to make sure that it compiles even without the new SDK. (But without HDR detection.) BUG=682416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
On 2017/06/26 19:07:47, palmer wrote: > Your CL description looks like it got truncated. :) Also "APi" -> "API". Fixed.
IPC security LGTM, with note. https://codereview.chromium.org/2938543002/diff/80001/gpu/ipc/service/direct_... File gpu/ipc/service/direct_composition_surface_win.cc (right): https://codereview.chromium.org/2938543002/diff/80001/gpu/ipc/service/direct_... gpu/ipc/service/direct_composition_surface_win.cc:1015: unsigned int i = 0; The style guide says not to use unsigned as a way to ensure integers aren't negative. If this is a regular counter, I guess it should be a signed int? If it's a size or array index, it should be a size_t. But, if |EnumOutputs| wants an unsigned int, it's OK for now. I know that changing those interfaces can sometimes turn into a huge CL. :)
https://codereview.chromium.org/2938543002/diff/80001/gpu/ipc/service/direct_... File gpu/ipc/service/direct_composition_surface_win.cc (right): https://codereview.chromium.org/2938543002/diff/80001/gpu/ipc/service/direct_... gpu/ipc/service/direct_composition_surface_win.cc:1015: unsigned int i = 0; On 2017/06/26 19:16:41, palmer wrote: > The style guide says not to use unsigned as a way to ensure integers aren't > negative. If this is a regular counter, I guess it should be a signed int? If > it's a size or array index, it should be a size_t. > > But, if |EnumOutputs| wants an unsigned int, it's OK for now. I know that > changing those interfaces can sometimes turn into a huge CL. :) EnumOutputs is a microsoft API, so changing it seems unlikely.
The CQ bit was checked by hubbe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/26 19:21:21, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Sorry for the late comment in the game. Would it make more sense to populate this in the display::Display at https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?rcl=7d383c8... Or is this needed in addition to that?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hubbe@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson for histograms
That seems like a good place to start.
It's needed in a few other places, which I will have to figure out how to
deal with.
/Hubbe
On Mon, Jun 26, 2017 at 12:27 PM, <ccameron@chromium.org> wrote:
> On 2017/06/26 19:21:21, commit-bot: I haz the power wrote:
> > CQ is trying da patch.
> >
> > Follow status at:
> >
> https://chromium-cq-status.appspot.com/v2/patch-status/
> codereview.chromium.org/2938543002/80001
>
> Sorry for the late comment in the game.
>
> Would it make more sense to populate this in the display::Display at
> https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?rcl=
> 7d383c8584f0dde579e78f7a47f17fd14b227311&l=94
>
> Or is this needed in addition to that?
>
> https://codereview.chromium.org/2938543002/
>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:8706: +<enum name="DXGI_COLOR_SPACE_TYPE" type="int"> It looks like these come from a third-party .h. Can you guarantee the values will never change? Usually in cases like this people built a translation layer from the third-party enum to a stable enum that can be used for UMA. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23544: + Records if the output color space of the monitor as reported by DXGI. "if" is that typo? Should it be removed? https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23545: + Recorded when the gpu process starts. It looks like this is recorded in a loop, and can be emitted multiple times. Yet this description only implies it's emitted once (exactly once). It's also not emitted if (FAILED(dxgi_adapter->EnumOutputs(i++, output.GetAddressOf()))) So something in this description needs to be revised. https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23562: + Records if the display maximum lumens as reported by windows. Uh, the other descriptions don't mention Windows. And they're emitted in exactly the same function. These descriptions should agree.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:8706: +<enum name="DXGI_COLOR_SPACE_TYPE" type="int"> On 2017/06/26 20:40:11, Mark P wrote: > It looks like these come from a third-party .h. Can you guarantee the values > will never change? Usually in cases like this people built a translation layer > from the third-party enum to a stable enum that can be used for UMA. > > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... These come from a microsoft API, which may be extended, but existing values cannot change without breaking compatibility. https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23544: + Records if the output color space of the monitor as reported by DXGI. On 2017/06/26 20:40:11, Mark P wrote: > "if" > is that typo? Should it be removed? Done. https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23545: + Recorded when the gpu process starts. On 2017/06/26 20:40:11, Mark P wrote: > It looks like this is recorded in a loop, and can be emitted multiple times. > Yet this description only implies it's emitted once (exactly once). > > It's also not emitted if > (FAILED(dxgi_adapter->EnumOutputs(i++, output.GetAddressOf()))) > > So something in this description needs to be revised. EnumOutputs really isn't expected to fail. However, I updated the code and the comment to reflect that these are recorded for each monitor. https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23562: + Records if the display maximum lumens as reported by windows. On 2017/06/26 20:40:11, Mark P wrote: > Uh, the other descriptions don't mention Windows. And they're emitted in > exactly the same function. These descriptions should agree. > I updated GPU.Output.ColorSpace GPU.Output.HDR is generic enough to be used on other platforms, when they add HDR support...
https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:8706: +<enum name="DXGI_COLOR_SPACE_TYPE" type="int"> On 2017/06/26 21:01:21, hubbe wrote: > On 2017/06/26 20:40:11, Mark P wrote: > > It looks like these come from a third-party .h. Can you guarantee the values > > will never change? Usually in cases like this people built a translation > layer > > from the third-party enum to a stable enum that can be used for UMA. > > > > > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > These come from a microsoft API, which may be extended, but existing values > cannot change without breaking compatibility. Acknowledged. https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23545: + Recorded when the gpu process starts. On 2017/06/26 21:01:21, hubbe wrote: > On 2017/06/26 20:40:11, Mark P wrote: > > It looks like this is recorded in a loop, and can be emitted multiple times. > > Yet this description only implies it's emitted once (exactly once). > > > > It's also not emitted if > > (FAILED(dxgi_adapter->EnumOutputs(i++, output.GetAddressOf()))) > > > > So something in this description needs to be revised. > > EnumOutputs really isn't expected to fail. Should you have a CHECK() / LOG(FATAL) in the code then? Or, if you think it can fail, at least an UMA_HISTOGRAM to count how often it fails versus succeeds. > However, I updated the code and the comment to reflect that these are recorded > for each monitor. https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:8717: +<enum name="DXGI_COLOR_SPACE_TYPE" type="int"> Please point in a xml comment to a reference to the third-party API where these are defined. https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23557: + Records the output color space of the monitor as reported by windows. nit: windows -> Windows ditto bottom histogram. https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23567: + starts. Please revise. "the output monitor". There may be more than one. Also, perhaps add: "Only recorded on Windows as of M-61." or something, to make it clear that it's reasonable at least for now to compare the counts and values of this to the other GPU histograms you're adding.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23545: + Recorded when the gpu process starts. On 2017/06/26 21:30:59, Mark P wrote: > On 2017/06/26 21:01:21, hubbe wrote: > > On 2017/06/26 20:40:11, Mark P wrote: > > > It looks like this is recorded in a loop, and can be emitted multiple times. > > > > Yet this description only implies it's emitted once (exactly once). > > > > > > It's also not emitted if > > > (FAILED(dxgi_adapter->EnumOutputs(i++, output.GetAddressOf()))) > > > > > > So something in this description needs to be revised. > > > > EnumOutputs really isn't expected to fail. > > Should you have a CHECK() / LOG(FATAL) in the code then? Or, if you think it > can fail, at least an UMA_HISTOGRAM to count how often it fails versus succeeds. > > > However, I updated the code and the comment to reflect that these are recorded > > for each monitor. > If we put in a uma histogram for every windows api that *can* fail but probably won't we won't have room for any real data. I mean: I can totally do that, but that's not usually how we write code for windows. If this fails, it's not a fatal error, we just won't enable HDR output. https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:8717: +<enum name="DXGI_COLOR_SPACE_TYPE" type="int"> On 2017/06/26 21:30:59, Mark P wrote: > Please point in a xml comment to a reference to the third-party API where these > are defined. Done. https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23557: + Records the output color space of the monitor as reported by windows. On 2017/06/26 21:30:59, Mark P wrote: > nit: windows -> Windows > ditto bottom histogram. Done. https://codereview.chromium.org/2938543002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23567: + starts. On 2017/06/26 21:30:59, Mark P wrote: > Please revise. "the output monitor". There may be more than one. > > Also, perhaps add: "Only recorded on Windows as of M-61." or something, to make > it clear that it's reasonable at least for now to compare the counts and values > of this to the other GPU histograms you're adding. > Done.
xml files lgtm modulo one request --mark https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23545: + Recorded when the gpu process starts. On 2017/06/26 22:08:58, hubbe wrote: > On 2017/06/26 21:30:59, Mark P wrote: > > On 2017/06/26 21:01:21, hubbe wrote: > > > On 2017/06/26 20:40:11, Mark P wrote: > > > > It looks like this is recorded in a loop, and can be emitted multiple > times. > > > > > > Yet this description only implies it's emitted once (exactly once). > > > > > > > > It's also not emitted if > > > > (FAILED(dxgi_adapter->EnumOutputs(i++, output.GetAddressOf()))) > > > > > > > > So something in this description needs to be revised. > > > > > > EnumOutputs really isn't expected to fail. > > > > Should you have a CHECK() / LOG(FATAL) in the code then? Or, if you think it > > can fail, at least an UMA_HISTOGRAM to count how often it fails versus > succeeds. > > > > > However, I updated the code and the comment to reflect that these are > recorded > > > for each monitor. > > > > If we put in a uma histogram for every windows api that *can* fail but probably > won't we won't have room for any real data. I mean: I can totally do that, but > that's not usually how we write code for windows. > > If this fails, it's not a fatal error, we just won't enable HDR output. > The histogram description should be clear then that data can be missing. E.g. Excludes monitors on which Windows failed to provide a description of the monitor.
https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2938543002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23545: + Recorded when the gpu process starts. On 2017/06/26 22:17:48, Mark P wrote: > On 2017/06/26 22:08:58, hubbe wrote: > > On 2017/06/26 21:30:59, Mark P wrote: > > > On 2017/06/26 21:01:21, hubbe wrote: > > > > On 2017/06/26 20:40:11, Mark P wrote: > > > > > It looks like this is recorded in a loop, and can be emitted multiple > > times. > > > > > > > > Yet this description only implies it's emitted once (exactly once). > > > > > > > > > > It's also not emitted if > > > > > (FAILED(dxgi_adapter->EnumOutputs(i++, output.GetAddressOf()))) > > > > > > > > > > So something in this description needs to be revised. > > > > > > > > EnumOutputs really isn't expected to fail. > > > > > > Should you have a CHECK() / LOG(FATAL) in the code then? Or, if you think > it > > > can fail, at least an UMA_HISTOGRAM to count how often it fails versus > > succeeds. > > > > > > > However, I updated the code and the comment to reflect that these are > > recorded > > > > for each monitor. > > > > > > > If we put in a uma histogram for every windows api that *can* fail but > probably > > won't we won't have room for any real data. I mean: I can totally do that, but > > that's not usually how we write code for windows. > > > > If this fails, it's not a fatal error, we just won't enable HDR output. > > > > The histogram description should be clear then that data can be missing. E.g. > Excludes monitors on which Windows failed to provide a description of the > monitor. Seems a little redundant, but ok.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, jbauman@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2938543002/#ps160001 (title: "histograms updated")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1498521862757680,
"parent_rev": "d211344c0b6f619d6cb1f86ad721ea2e314399ad", "commit_rev":
"b9ab2095d1f334506aa74681fa758adf03ab4a13"}
Message was sent while issue was closed.
Description was changed from ========== Detect HDR capability This calls the windows API for detecting if the output display can do HDR output or not. Note that this API depends on a recent version of the WinSDK, but has been guarded by ifdefs to make sure that it compiles even without the new SDK. (But without HDR detection.) BUG=682416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Detect HDR capability This calls the windows API for detecting if the output display can do HDR output or not. Note that this API depends on a recent version of the WinSDK, but has been guarded by ifdefs to make sure that it compiles even without the new SDK. (But without HDR detection.) BUG=682416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2938543002 Cr-Commit-Position: refs/heads/master@{#482479} Committed: https://chromium.googlesource.com/chromium/src/+/b9ab2095d1f334506aa74681fa75... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/b9ab2095d1f334506aa74681fa75...
Message was sent while issue was closed.
This patch is causing a build warning when I try building locally on win10, which makes build fail (warnings treated as error): d:\eseckler\chromium\src\gpu\ipc\service\direct_composition_surface_win.cc(1002): error C2220: warning treated as error - no 'object' file generated d:\eseckler\chromium\src\gpu\ipc\service\direct_composition_surface_win.cc(1002): warning C4189: 'hdr_monitor_found': local variable is initialized but not referenced looks like the declaration/definition of hdr_monitor_found should be inside the ifdef...
Message was sent while issue was closed.
Apologies, will fix later today when I get to work. On Tue, Jun 27, 2017 at 7:02 AM, <eseckler@chromium.org> wrote: > This patch is causing a build warning when I try building locally on win10, > which makes build fail (warnings treated as error): > > d:\eseckler\chromium\src\gpu\ipc\service\direct_ > composition_surface_win.cc(1002): > error C2220: warning treated as error - no 'object' file generated > d:\eseckler\chromium\src\gpu\ipc\service\direct_ > composition_surface_win.cc(1002): > warning C4189: 'hdr_monitor_found': local variable is initialized but not > referenced > > looks like the declaration/definition of hdr_monitor_found should be > inside the > ifdef... > > https://codereview.chromium.org/2938543002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
