|
|
Created:
3 years, 5 months ago by Hzj_jie Modified:
3 years, 5 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice
DesktopCaptureOptions impacts which underlying DesktopCapturer implementation is
used. But there is no guarantee that different DesktopCapturer implementations
would eventually return the same SourceList, or a Source in the SourceList
represents the same output content on the system. So DesktopCaptureBase and
DesktopCaptureDevice should use same DesktopCaptureOptions.
BUG=732224
Review-Url: https://codereview.chromium.org/2961193002
Cr-Commit-Position: refs/heads/master@{#485097}
Committed: https://chromium.googlesource.com/chromium/src/+/56efe642b5d1fc400ac7f34451d9a540f9d30fbb
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments #Patch Set 3 : Move CreateDesktopCaptureOptions into content/public/browser #Patch Set 4 : CONTENT_EXPORT #Patch Set 5 : Sync latest changes #
Messages
Total messages: 75 (59 generated)
Description was changed from ========== Use same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice DesktopCaptureBase and DesktopCaptureDevice should use same DesktopCaptureOptions. BUG=732224 ========== to ========== Use same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice DesktopCaptureBase and DesktopCaptureDevice should use same DesktopCaptureOptions. BUG=732224 ==========
zijiehe@chromium.org changed reviewers: + niklase@chromium.org
The CQ bit was checked by zijiehe@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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.
Conceptually this looks good to me, we should definitely use the same experiment in both cases, but someone that knows the code better should review too. Any way we can avoid the code duplication?
On 2017/06/29 22:32:28, Niklas Enbom wrote: > Conceptually this looks good to me, we should definitely use the same experiment > in both cases, but someone that knows the code better should review too. Any way > we can avoid the code duplication? It looks like there is no direct dependency between content/browser and chrome/browser. I have considered content/public, but nothing related to desktop capturer has been placed there. Since this configuration is rarely changed, this level of duplication seems not really unacceptable. I will ask Sergey to also review this change.
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Sergeyu, this change ensures a same DesktopCaptureOptions is used in both picker and capturer. Please have a look. Thank you.
Is the issue that different capturers identify multiple monitors differently? It's not clear how this change is related to the bug. https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... Instead of duplicating the code here, can this file just call the same function that's used in content/browser/media/capture/desktop_capture_device.cc? Both files are in content/browser, so it should be possible to share code between them. https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:65: #endif // defined(OS_WIN) https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:68: #endif // defined(OS_ASH)
Description was changed from ========== Use same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice DesktopCaptureBase and DesktopCaptureDevice should use same DesktopCaptureOptions. BUG=732224 ========== to ========== Use same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice DesktopCaptureOptions impacts which underlying DesktopCapturer implementation is used. But there is no guarantee that different DesktopCapturer implementations would eventually return the same SourceList, or a Source in the SourceList represents the same output content on the system. So DesktopCaptureBase and DesktopCaptureDevice should use same DesktopCaptureOptions. BUG=732224 ==========
The CQ bit was checked by zijiehe@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/2961193002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... On 2017/06/29 23:49:01, Sergey Ulanov wrote: > Instead of duplicating the code here, can this file just call the same function > that's used in content/browser/media/capture/desktop_capture_device.cc? > Both files are in content/browser, so it should be possible to share code > between them. It's do a little bit confusing, but one file is in chrome/browser, the other is in content/browser :) https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:65: #endif On 2017/06/29 23:49:01, Sergey Ulanov wrote: > // defined(OS_WIN) Done. https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:68: #endif On 2017/06/29 23:49:01, Sergey Ulanov wrote: > // defined(OS_ASH) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Any other comments?
https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... On 2017/06/29 23:56:27, Hzj_jie wrote: > On 2017/06/29 23:49:01, Sergey Ulanov wrote: > > Instead of duplicating the code here, can this file just call the same > function > > that's used in content/browser/media/capture/desktop_capture_device.cc? > > Both files are in content/browser, so it should be possible to share code > > between them. > > It's do a little bit confusing, but one file is in chrome/browser, the other is > in content/browser :) Sorry I missed that. Maybe put this function in webrtc? I think you should still be able to get the state of DirectX feature using webrtc::field_trial::IsEnabled(). If that doesn't work then I'd prefer exposing this function in content/public.
The CQ bit was checked by zijiehe@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by zijiehe@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by zijiehe@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #3 (id:120001) has been deleted
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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by zijiehe@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 failures do not relate to my change. https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... On 2017/07/05 19:31:07, Sergey Ulanov wrote: > On 2017/06/29 23:56:27, Hzj_jie wrote: > > On 2017/06/29 23:49:01, Sergey Ulanov wrote: > > > Instead of duplicating the code here, can this file just call the same > > function > > > that's used in content/browser/media/capture/desktop_capture_device.cc? > > > Both files are in content/browser, so it should be possible to share code > > > between them. > > > > It's do a little bit confusing, but one file is in chrome/browser, the other > is > > in content/browser :) > > Sorry I missed that. Maybe put this function in webrtc? I think you should still > be able to get the state of DirectX feature using > webrtc::field_trial::IsEnabled(). If that doesn't work then I'd prefer exposing > this function in content/public. Placing this logic in webrtc seems not a good idea to me. Chromoting does not rely on Finch at all. But I can move them into content/public/browser/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This change LGTM, but I'm not content/public OWNER. On 2017/07/07 01:45:08, Hzj_jie wrote: > The failures do not relate to my change. > > https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... > File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc > (right): > > https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... > chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // > https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... > On 2017/07/05 19:31:07, Sergey Ulanov wrote: > > On 2017/06/29 23:56:27, Hzj_jie wrote: > > > On 2017/06/29 23:49:01, Sergey Ulanov wrote: > > > > Instead of duplicating the code here, can this file just call the same > > > function > > > > that's used in content/browser/media/capture/desktop_capture_device.cc? > > > > Both files are in content/browser, so it should be possible to share code > > > > between them. > > > > > > It's do a little bit confusing, but one file is in chrome/browser, the other > > is > > > in content/browser :) > > > > Sorry I missed that. Maybe put this function in webrtc? I think you should > still > > be able to get the state of DirectX feature using > > webrtc::field_trial::IsEnabled(). If that doesn't work then I'd prefer > exposing > > this function in content/public. > > Placing this logic in webrtc seems not a good idea to me. Chromoting does not > rely on Finch at all. Not sure why it would affect Chromoting. The host wouldn't have to use this function - it can construct DesktopCaptureOptions the way it currently does. > But I can move them into content/public/browser/.
On 2017/07/07 17:35:12, Sergey Ulanov wrote: > This change LGTM, but I'm not content/public OWNER. > > On 2017/07/07 01:45:08, Hzj_jie wrote: > > The failures do not relate to my change. > > > > > https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... > > File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc > > (right): > > > > > https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... > > chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // > > > https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... > > On 2017/07/05 19:31:07, Sergey Ulanov wrote: > > > On 2017/06/29 23:56:27, Hzj_jie wrote: > > > > On 2017/06/29 23:49:01, Sergey Ulanov wrote: > > > > > Instead of duplicating the code here, can this file just call the same > > > > function > > > > > that's used in content/browser/media/capture/desktop_capture_device.cc? > > > > > Both files are in content/browser, so it should be possible to share > code > > > > > between them. > > > > > > > > It's do a little bit confusing, but one file is in chrome/browser, the > other > > > is > > > > in content/browser :) > > > > > > Sorry I missed that. Maybe put this function in webrtc? I think you should > > still > > > be able to get the state of DirectX feature using > > > webrtc::field_trial::IsEnabled(). If that doesn't work then I'd prefer > > exposing > > > this function in content/public. > > > > Placing this logic in webrtc seems not a good idea to me. Chromoting does not > > rely on Finch at all. > > Not sure why it would affect Chromoting. The host wouldn't have to use this > function - it can construct DesktopCaptureOptions the way it currently does. Sorry for the unclear explanation. I think placing a Chrome specific implementation in WebRTC is not proper, especially Chromoting, the other user of this component, does not share the same logic as Chrome. > > > > But I can move them into content/public/browser/.
On 2017/07/07 17:39:28, Hzj_jie wrote: > On 2017/07/07 17:35:12, Sergey Ulanov wrote: > > This change LGTM, but I'm not content/public OWNER. > > > > On 2017/07/07 01:45:08, Hzj_jie wrote: > > > The failures do not relate to my change. > > > > > > > > > https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... > > > File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2961193002/diff/20001/chrome/browser/extensio... > > > chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:50: // > > > > > > https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... > > > On 2017/07/05 19:31:07, Sergey Ulanov wrote: > > > > On 2017/06/29 23:56:27, Hzj_jie wrote: > > > > > On 2017/06/29 23:49:01, Sergey Ulanov wrote: > > > > > > Instead of duplicating the code here, can this file just call the same > > > > > function > > > > > > that's used in > content/browser/media/capture/desktop_capture_device.cc? > > > > > > Both files are in content/browser, so it should be possible to share > > code > > > > > > between them. > > > > > > > > > > It's do a little bit confusing, but one file is in chrome/browser, the > > other > > > > is > > > > > in content/browser :) > > > > > > > > Sorry I missed that. Maybe put this function in webrtc? I think you should > > > still > > > > be able to get the state of DirectX feature using > > > > webrtc::field_trial::IsEnabled(). If that doesn't work then I'd prefer > > > exposing > > > > this function in content/public. > > > > > > Placing this logic in webrtc seems not a good idea to me. Chromoting does > not > > > rely on Finch at all. > > > > Not sure why it would affect Chromoting. The host wouldn't have to use this > > function - it can construct DesktopCaptureOptions the way it currently does. > Sorry for the unclear explanation. I think placing a Chrome specific > implementation in WebRTC is not proper, especially Chromoting, the other user of > this component, does not share the same logic as Chrome. > > > > > > > But I can move them into content/public/browser/. content/public lgtm
The CQ bit was checked by zijiehe@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...)
The CQ bit was checked by zijiehe@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 zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2961193002/#ps180001 (title: "Sync latest changes")
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": 180001, "attempt_start_ts": 1499469664262310, "parent_rev": "83b3cdfd66d9ce73d5f4a8a0d4987d32f2ff0e5b", "commit_rev": "56efe642b5d1fc400ac7f34451d9a540f9d30fbb"}
Message was sent while issue was closed.
Description was changed from ========== Use same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice DesktopCaptureOptions impacts which underlying DesktopCapturer implementation is used. But there is no guarantee that different DesktopCapturer implementations would eventually return the same SourceList, or a Source in the SourceList represents the same output content on the system. So DesktopCaptureBase and DesktopCaptureDevice should use same DesktopCaptureOptions. BUG=732224 ========== to ========== Use same DesktopCaptureOptions in DesktopCaptureBase and DesktopCaptureDevice DesktopCaptureOptions impacts which underlying DesktopCapturer implementation is used. But there is no guarantee that different DesktopCapturer implementations would eventually return the same SourceList, or a Source in the SourceList represents the same output content on the system. So DesktopCaptureBase and DesktopCaptureDevice should use same DesktopCaptureOptions. BUG=732224 Review-Url: https://codereview.chromium.org/2961193002 Cr-Commit-Position: refs/heads/master@{#485097} Committed: https://chromium.googlesource.com/chromium/src/+/56efe642b5d1fc400ac7f34451d9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/56efe642b5d1fc400ac7f34451d9... |