Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801193003:
Add feature flag to convert BT.709 to BT.601 video in overlays. (Closed)
DescriptionAdd feature flag to convert BT.709 to BT.601 video in overlays.
Some systems seem to be ignoring the BT.709 colorspace, so add a feature
flag so we can test converting it to BT.601 in the video processor so
it will display properly.
BUG=678800
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/2801193003
Cr-Commit-Position: refs/heads/master@{#463842}
Committed: https://chromium.googlesource.com/chromium/src/+/137707cfcb43771e8a3d6805c3f327c22a4f87d9
Patch Set 1 #
Total comments: 6
Patch Set 2 : some changes #Messages
Total messages: 19 (13 generated)
Description was changed from ========== Add feature flag to convert BT.709 to BT.601 video in overlays. Some systems seem to be ignoring the BT.709 colorspace, so add a feature flag so we can test converting it to BT.601 in the video processor so it will display properly. BUG=678800 ========== to ========== Add feature flag to convert BT.709 to BT.601 video in overlays. Some systems seem to be ignoring the BT.709 colorspace, so add a feature flag so we can test converting it to BT.601 in the video processor so it will display properly. BUG=678800 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 jbauman@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...
jbauman@chromium.org changed reviewers: + sunnyps@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2801193003/diff/1/gpu/ipc/service/direct_comp... File gpu/ipc/service/direct_composition_surface_win.cc (right): https://codereview.chromium.org/2801193003/diff/1/gpu/ipc/service/direct_comp... gpu/ipc/service/direct_composition_surface_win.cc:49: const base::Feature kConvertVideoToBT601{"ConvertVideoToBT601", nit: Can the name be something like FallbackBT709VideoToBT601? https://codereview.chromium.org/2801193003/diff/1/gpu/ipc/service/direct_comp... gpu/ipc/service/direct_composition_surface_win.cc:342: if (SUCCEEDED(video_context_.QueryInterface(context1.Receive()))) { Can you explain in a comment the difference between setting a D3D11 color space vs a DXGI color space? https://codereview.chromium.org/2801193003/diff/1/gpu/ipc/service/direct_comp... gpu/ipc/service/direct_composition_surface_win.cc:355: if (base::FeatureList::IsEnabled(kConvertVideoToBT601) && If the FeatureList singleton isn't initialized, IsEnabled just returns the default value. I can't find any place where FeatureList is initialized in GPU process. I think you'll have to plumb this all the way from the browser. https://codereview.chromium.org/2801193003/diff/1/gpu/ipc/service/direct_comp... gpu/ipc/service/direct_composition_surface_win.cc:360: base::win::ScopedComPtr<IDXGISwapChain3> swap_chain3; It doesn't seem like you can IDXGISwapChain3 but not ID3D11VideoContext1. Both are on Windows 10 only. I don't think you need the D3D11 color space special cases either because you can use DXGI everywhere (after checking of course).
https://codereview.chromium.org/2801193003/diff/1/gpu/ipc/service/direct_comp... File gpu/ipc/service/direct_composition_surface_win.cc (right): https://codereview.chromium.org/2801193003/diff/1/gpu/ipc/service/direct_comp... gpu/ipc/service/direct_composition_surface_win.cc:355: if (base::FeatureList::IsEnabled(kConvertVideoToBT601) && On 2017/04/11 21:27:23, sunnyps wrote: > If the FeatureList singleton isn't initialized, IsEnabled just returns the > default value. I can't find any place where FeatureList is initialized in GPU > process. I think you'll have to plumb this all the way from the browser. This is initialized in InitializeFieldTrialAndFeatureList in content/app/content_main_runner.cc https://codereview.chromium.org/2801193003/diff/1/gpu/ipc/service/direct_comp... gpu/ipc/service/direct_composition_surface_win.cc:360: base::win::ScopedComPtr<IDXGISwapChain3> swap_chain3; On 2017/04/11 21:27:23, sunnyps wrote: > It doesn't seem like you can IDXGISwapChain3 but not ID3D11VideoContext1. Both > are on Windows 10 only. I don't think you need the D3D11 color space special > cases either because you can use DXGI everywhere (after checking of course). Just because it's first available in Windows 10 doesn't mean that it's always supported. It may be gated on hardware support/wddm version - for example I think you need WDDM 2.0 for ID3D11VideoContext1, but I don't see any reason you'd need it for IDXGISwapChain3. Also they could be added in different versions of windows 10, but I think in this case they're both at least in the anniversary update.
The CQ bit was checked by jbauman@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...
lgtm
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 jbauman@chromium.org
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": 20001, "attempt_start_ts": 1491953757355090,
"parent_rev": "8152d2adcabba8e58d47308f233f2619230fe711", "commit_rev":
"137707cfcb43771e8a3d6805c3f327c22a4f87d9"}
Message was sent while issue was closed.
Description was changed from ========== Add feature flag to convert BT.709 to BT.601 video in overlays. Some systems seem to be ignoring the BT.709 colorspace, so add a feature flag so we can test converting it to BT.601 in the video processor so it will display properly. BUG=678800 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 ========== Add feature flag to convert BT.709 to BT.601 video in overlays. Some systems seem to be ignoring the BT.709 colorspace, so add a feature flag so we can test converting it to BT.601 in the video processor so it will display properly. BUG=678800 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/2801193003 Cr-Commit-Position: refs/heads/master@{#463842} Committed: https://chromium.googlesource.com/chromium/src/+/137707cfcb43771e8a3d6805c3f3... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/137707cfcb43771e8a3d6805c3f3... |
