Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1576073003:
Support Audio for Desktop Capture (Closed)
|
Created:
4 years, 11 months ago by qiangchen Modified:
4 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, miu+watch_chromium.org, msramek, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport Audio for Desktop Capture
In this CL, we did the following stuff:
Stream audio with screen share (only supported for
platform Windows and ChromeOS). The essential
functionality was there, we just connect the code
path.
Currently we put the functionalities behind a flag
--enable-audio-support-for-desktop-share.
We'll bring the functionality on by default, after we
finish the future CL which handles tab sharing and the
future CL which handles the user permission for audio
output sharing.
BUG=557222
Committed: https://crrev.com/84b640a130802c7159fd66254ce9c69d0aad51ed
Cr-Commit-Position: refs/heads/master@{#371580}
Patch Set 1 : Support loopback audio #Patch Set 2 : User Permission #Patch Set 3 : Rebase #
Total comments: 10
Patch Set 4 : Style Fix #
Total comments: 4
Patch Set 5 : Same as patch 1 #
Messages
Total messages: 34 (18 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Description was changed from ========== Support Audio for Desktop Capture Currently we put the functionality behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles security request for system audio and the future CL which handles desktop media picker UI. BUG=557222 ========== to ========== Support Audio for Desktop Capture In this CL, we did the following stuff: 1. Stream audio with screen share (only supported for platform Windows and ChromeOS) 2. In the Desktop Share Window Picker, add a checkbox which is essentially asking the user's permission for audio sharing. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which improves desktop media picker UI. BUG=557222 ==========
Description was changed from ========== Support Audio for Desktop Capture In this CL, we did the following stuff: 1. Stream audio with screen share (only supported for platform Windows and ChromeOS) 2. In the Desktop Share Window Picker, add a checkbox which is essentially asking the user's permission for audio sharing. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which improves desktop media picker UI. BUG=557222 ========== to ========== Support Audio for Desktop Capture In this CL, we did the following stuff: 1. Stream audio with screen share (only supported for platform Windows and ChromeOS) 2. In the Desktop Share Window Picker, add a checkbox which is essentially asking the user's permission for audio sharing. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which improves desktop media picker UI. BUG=557222 ==========
Patchset #2 (id:120001) has been deleted
qiangchen@chromium.org changed reviewers: + isherman@chromium.org, jochen@chromium.org
Hi, jochen@: PTAL. The main functionality of loopback audio (namely capturing system audio) is there. This CL connects it with Desktop share, so that we can make screen share with sound. Besides, I add a checkbox on the picker window, to notify the end user and allow them to choose whether to allow audio sharing. However, the UI to do this is negotiable, but not the point of this CL. We've planned to deal with the picker window UI in other CLs. See the design for detail. And I've put everything behind a flag. Hi, isherman@: Please focus on the xml file change. As I added a value to ContentSettingsType enum.
histograms.xml lgtm
qiangchen@chromium.org changed reviewers: + gyzhou@chromium.org
jochen@chromium.org changed reviewers: + raymes@chromium.org
I don't have context for this CL. audio access & content settings sounds like something raymes@ might know?
lgtm Some minor stuff. https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/media/d... File chrome/browser/media/desktop_capture_access_handler.cc (right): https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/media/d... chrome/browser/media/desktop_capture_access_handler.cc:349: bool loopback_audio_permitted = true; Maybe use this flag for all audio share (such as tab audio) change to bool audio_permitted = true; https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:399: web_contents_(parent_web_contents), parent_web_contents_(parent_web_contents) probably is clearer. https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:405: l10n_util::GetStringUTF16(IDS_DESTOP_MEDIA_PICKER_ALLOW_AUDIO))) { Spell error. Change to IDS_DESKTOP_MEDIA_PICKER_ALLOW_AUDIO or IDS_DESKTOP_MEDIA_PICKER_ALLOW_AUDIO_SHARE https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:427: GURL origin(parent_web_contents->GetLastCommittedURL().GetOrigin()); Is it possible change to const GURL? https://codereview.chromium.org/1576073003/diff/150001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1576073003/diff/150001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58780: + <int value="26" label="Media setting (desktop audio)"/> Is there any reason that number jumps from 22 to 26?
lgtm Some minor stuff.
https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/media/d... File chrome/browser/media/desktop_capture_access_handler.cc (right): https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/media/d... chrome/browser/media/desktop_capture_access_handler.cc:349: bool loopback_audio_permitted = true; On 2016/01/15 16:37:55, GeorgeZ wrote: > Maybe use this flag for all audio share (such as tab audio) > change to > bool audio_permitted = true; Done. https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:399: web_contents_(parent_web_contents), On 2016/01/15 16:37:55, GeorgeZ wrote: > parent_web_contents_(parent_web_contents) probably is clearer. Done. https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:405: l10n_util::GetStringUTF16(IDS_DESTOP_MEDIA_PICKER_ALLOW_AUDIO))) { On 2016/01/15 16:37:55, GeorgeZ wrote: > Spell error. Change to IDS_DESKTOP_MEDIA_PICKER_ALLOW_AUDIO or > IDS_DESKTOP_MEDIA_PICKER_ALLOW_AUDIO_SHARE Done. https://codereview.chromium.org/1576073003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:427: GURL origin(parent_web_contents->GetLastCommittedURL().GetOrigin()); On 2016/01/15 16:37:55, GeorgeZ wrote: > Is it possible change to const GURL? Done. https://codereview.chromium.org/1576073003/diff/150001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1576073003/diff/150001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58780: + <int value="26" label="Media setting (desktop audio)"/> On 2016/01/15 16:37:55, GeorgeZ wrote: > Is there any reason that number jumps from 22 to 26? The value must match the position in kHistogramOrder in content_settings.cc The value for 23, 24, 25 were not added into histograms.xml
https://codereview.chromium.org/1576073003/diff/170001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1576073003/diff/170001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:265: Register(CONTENT_SETTINGS_TYPE_DESKTOP_AUDIO_SHARE, "desktop-audio-share", Could you please explain why the content setting is needed? Will there be UI around this?
Hi, raymes@ Replied to your question. Qiang https://codereview.chromium.org/1576073003/diff/170001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1576073003/diff/170001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:265: Register(CONTENT_SETTINGS_TYPE_DESKTOP_AUDIO_SHARE, "desktop-audio-share", On 2016/01/17 23:27:34, raymes wrote: > Could you please explain why the content setting is needed? Will there be UI > around this? Sure. We are going to attach the audio stream when doing desktop share. Namely, attach the system audio when doing screen share, and in the future attach the tab audio when doing tab share. We also notice that sharing audio may not be noticeable by the end user. Due to privacy issue, we decide to ask user permission if the website wants to share audio when doing desktop share. I just think the permission is similar to the permission for microphone access, and thus add a CONTENT_SETTING_TYPE. There is a UI about this. Currently, I add a checkbox on Desktop Share Window Picker say "Allow audio sharing". Using permission bubble can be an alternative way to do so, but there is some UI difficulty: when sharing a tab, we will activate the target tab to be shared, but the permission bubble is associated with the original tab that initiates the sharing. Or we may come up with other ideas on how to ask for user permission. Anyway, the UI is not the point of this CL. According to the project plan, we will have separate CLs focusing on the UI design. This CL is mainly doing the backend support for audio sharing with desktop share.
msramek@chromium.org changed reviewers: + msramek@chromium.org
https://codereview.chromium.org/1576073003/diff/170001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1576073003/diff/170001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:265: Register(CONTENT_SETTINGS_TYPE_DESKTOP_AUDIO_SHARE, "desktop-audio-share", On 2016/01/19 17:33:22, qiangchenC wrote: > On 2016/01/17 23:27:34, raymes wrote: > > Could you please explain why the content setting is needed? Will there be UI > > around this? > > Sure. We are going to attach the audio stream when doing desktop share. Namely, > attach the system audio when doing screen share, and in the future attach the > tab audio when doing tab share. We also notice that sharing audio may not be > noticeable by the end user. Due to privacy issue, we decide to ask user > permission if the website wants to share audio when doing desktop share. I just > think the permission is similar to the permission for microphone access, and > thus add a CONTENT_SETTING_TYPE. > > There is a UI about this. Currently, I add a checkbox on Desktop Share Window > Picker say "Allow audio sharing". Using permission bubble can be an alternative > way to do so, but there is some UI difficulty: when sharing a tab, we will > activate the target tab to be shared, but the permission bubble is associated > with the original tab that initiates the sharing. Or we may come up with other > ideas on how to ask for user permission. Anyway, the UI is not the point of this > CL. According to the project plan, we will have separate CLs focusing on the UI > design. This CL is mainly doing the backend support for audio sharing with > desktop share. Drive by! Disclaimer: I'm not generally opposed to adding this setting, just playing Devil's advocate. Content settings like microphone are "sticky permissions" that generally express my trust to a website. For example, I absolutely don't want any website to listen to my microphone, except Hangouts, because I use that one often. So I'll create a mic content setting exception for Hangouts. "Share sound when screen sharing" sounds like something I would want to decide on a per-session basis rather than per-website. Especially if you go with the checkbox on the picker, content setting wouldn't be needed, right? If it's not a website-related setting that is exposed through the chrome://settings/content UI nor as an administration policy, it might not be a good fit for content setting. If this is going to be a user- (not just administrator-) settable content setting that will not be exposed in chrome://settings/content UI, I think it's going to be the very first case. Content settings are a form of history, so if there is no UI entrypoint to inspect which URLs are recorded and delete them, please make sure to clear the setting when the user deletes their history.
On 2016/01/19 18:02:20, msramek wrote: > https://codereview.chromium.org/1576073003/diff/170001/components/content_set... > File components/content_settings/core/browser/content_settings_registry.cc > (right): > > https://codereview.chromium.org/1576073003/diff/170001/components/content_set... > components/content_settings/core/browser/content_settings_registry.cc:265: > Register(CONTENT_SETTINGS_TYPE_DESKTOP_AUDIO_SHARE, "desktop-audio-share", > On 2016/01/19 17:33:22, qiangchenC wrote: > > On 2016/01/17 23:27:34, raymes wrote: > > > Could you please explain why the content setting is needed? Will there be UI > > > around this? > > > > Sure. We are going to attach the audio stream when doing desktop share. > Namely, > > attach the system audio when doing screen share, and in the future attach the > > tab audio when doing tab share. We also notice that sharing audio may not be > > noticeable by the end user. Due to privacy issue, we decide to ask user > > permission if the website wants to share audio when doing desktop share. I > just > > think the permission is similar to the permission for microphone access, and > > thus add a CONTENT_SETTING_TYPE. > > > > There is a UI about this. Currently, I add a checkbox on Desktop Share Window > > Picker say "Allow audio sharing". Using permission bubble can be an > alternative > > way to do so, but there is some UI difficulty: when sharing a tab, we will > > activate the target tab to be shared, but the permission bubble is associated > > with the original tab that initiates the sharing. Or we may come up with > other > > ideas on how to ask for user permission. Anyway, the UI is not the point of > this > > CL. According to the project plan, we will have separate CLs focusing on the > UI > > design. This CL is mainly doing the backend support for audio sharing with > > desktop share. > > Drive by! Disclaimer: I'm not generally opposed to adding this setting, just > playing Devil's advocate. > > Content settings like microphone are "sticky permissions" that generally express > my trust to a website. For example, I absolutely don't want any website to > listen to my microphone, except Hangouts, because I use that one often. So I'll > create a mic content setting exception for Hangouts. > > "Share sound when screen sharing" sounds like something I would want to decide > on a per-session basis rather than per-website. Especially if you go with the > checkbox on the picker, content setting wouldn't be needed, right? > > If it's not a website-related setting that is exposed through the > chrome://settings/content UI nor as an administration policy, it might not be a > good fit for content setting. > > If this is going to be a user- (not just administrator-) settable content > setting that will not be exposed in chrome://settings/content UI, I think it's > going to be the very first case. Content settings are a form of history, so if > there is no UI entrypoint to inspect which URLs are recorded and delete them, > please make sure to clear the setting when the user deletes their history. I'd agree with Martin above.Would it be reasonable to always ask the user (at least as a first-cut UI) which wouldn't require a content setting? My guess is that most users would want the default to be "no sound" even if they have been on that site before and shared sound. But that might be wrong :) If it turned out to be wrong you could consider adding a setting later on.
replied https://codereview.chromium.org/1576073003/diff/170001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1576073003/diff/170001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:265: Register(CONTENT_SETTINGS_TYPE_DESKTOP_AUDIO_SHARE, "desktop-audio-share", On 2016/01/19 18:02:20, msramek wrote: > On 2016/01/19 17:33:22, qiangchenC wrote: > > On 2016/01/17 23:27:34, raymes wrote: > > > Could you please explain why the content setting is needed? Will there be UI > > > around this? > > > > Sure. We are going to attach the audio stream when doing desktop share. > Namely, > > attach the system audio when doing screen share, and in the future attach the > > tab audio when doing tab share. We also notice that sharing audio may not be > > noticeable by the end user. Due to privacy issue, we decide to ask user > > permission if the website wants to share audio when doing desktop share. I > just > > think the permission is similar to the permission for microphone access, and > > thus add a CONTENT_SETTING_TYPE. > > > > There is a UI about this. Currently, I add a checkbox on Desktop Share Window > > Picker say "Allow audio sharing". Using permission bubble can be an > alternative > > way to do so, but there is some UI difficulty: when sharing a tab, we will > > activate the target tab to be shared, but the permission bubble is associated > > with the original tab that initiates the sharing. Or we may come up with > other > > ideas on how to ask for user permission. Anyway, the UI is not the point of > this > > CL. According to the project plan, we will have separate CLs focusing on the > UI > > design. This CL is mainly doing the backend support for audio sharing with > > desktop share. > > Drive by! Disclaimer: I'm not generally opposed to adding this setting, just > playing Devil's advocate. > > Content settings like microphone are "sticky permissions" that generally express > my trust to a website. For example, I absolutely don't want any website to > listen to my microphone, except Hangouts, because I use that one often. So I'll > create a mic content setting exception for Hangouts. > > "Share sound when screen sharing" sounds like something I would want to decide > on a per-session basis rather than per-website. Especially if you go with the > checkbox on the picker, content setting wouldn't be needed, right? > > If it's not a website-related setting that is exposed through the > chrome://settings/content UI nor as an administration policy, it might not be a > good fit for content setting. > > If this is going to be a user- (not just administrator-) settable content > setting that will not be exposed in chrome://settings/content UI, I think it's > going to be the very first case. Content settings are a form of history, so if > there is no UI entrypoint to inspect which URLs are recorded and delete them, > please make sure to clear the setting when the user deletes their history. Thanks for your comment. Several options in my mind: 1. Use checkbox on picker window VS Use permission bubble. Personally, I think permission bubble is more consistent with camera/mircophone access request. But one issue is for standalone extensions, the main UI is not a web page, and thus no place to show permission bubble. (Maybe this is not the big issue, because the main use of desktop share is still within a webpage) 2. Memorize user's selection VS Ask for permission every time. Personally, I think memorizing user selection will have better experience. Then the technical question is whether it is appropriate to use content settings. And as you mentioned, I will need to add a section in chrome://settings/content for the user to manage the white list. Asking every time is relative easier to implement, but may annoy the user to do the selection on the same website every time. WDYT? Or do you have an idea where can I store the user selection, if content setting is inappropriate place? I think I'll pend this CL and throw out a design doc to listen to more voices.
Description was changed from ========== Support Audio for Desktop Capture In this CL, we did the following stuff: 1. Stream audio with screen share (only supported for platform Windows and ChromeOS) 2. In the Desktop Share Window Picker, add a checkbox which is essentially asking the user's permission for audio sharing. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which improves desktop media picker UI. BUG=557222 ========== to ========== Support Audio for Desktop Capture In this CL, we did the following stuff: Stream audio with screen share (only supported for platform Windows and ChromeOS) Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which handles the user permission for audio output sharing. BUG=557222 ==========
Description was changed from ========== Support Audio for Desktop Capture In this CL, we did the following stuff: Stream audio with screen share (only supported for platform Windows and ChromeOS) Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which handles the user permission for audio output sharing. BUG=557222 ========== to ========== Support Audio for Desktop Capture In this CL, we did the following stuff: Stream audio with screen share (only supported for platform Windows and ChromeOS). The essential functionality was there, we just connect the code path. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which handles the user permission for audio output sharing. BUG=557222 ==========
After resolving the review comments in my design doc, the user permission is going to be handled by permission bubble. To make that completely usable, we also need to modify the Content Setting UI and Web Setting Popup UI. Squeezing everything in this CL is definitely too large. So I decide to split the workload into 4 light-weight CLs: 1. This one, add a flag to make the ongoing work invisible by default. And make audio stream generating code path connected with desktop share. 2. Content Setting UI 3. Website Setting Popup UI 4. Permission bubble for on-the-fly permission asking.
lgtm
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gyzhou@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1576073003/#ps190001 (title: "Same as patch 1")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576073003/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576073003/190001
Message was sent while issue was closed.
Description was changed from ========== Support Audio for Desktop Capture In this CL, we did the following stuff: Stream audio with screen share (only supported for platform Windows and ChromeOS). The essential functionality was there, we just connect the code path. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which handles the user permission for audio output sharing. BUG=557222 ========== to ========== Support Audio for Desktop Capture In this CL, we did the following stuff: Stream audio with screen share (only supported for platform Windows and ChromeOS). The essential functionality was there, we just connect the code path. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which handles the user permission for audio output sharing. BUG=557222 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Support Audio for Desktop Capture In this CL, we did the following stuff: Stream audio with screen share (only supported for platform Windows and ChromeOS). The essential functionality was there, we just connect the code path. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which handles the user permission for audio output sharing. BUG=557222 ========== to ========== Support Audio for Desktop Capture In this CL, we did the following stuff: Stream audio with screen share (only supported for platform Windows and ChromeOS). The essential functionality was there, we just connect the code path. Currently we put the functionalities behind a flag --enable-audio-support-for-desktop-share. We'll bring the functionality on by default, after we finish the future CL which handles tab sharing and the future CL which handles the user permission for audio output sharing. BUG=557222 Committed: https://crrev.com/84b640a130802c7159fd66254ce9c69d0aad51ed Cr-Commit-Position: refs/heads/master@{#371580} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/84b640a130802c7159fd66254ce9c69d0aad51ed Cr-Commit-Position: refs/heads/master@{#371580} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
