Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538033003:
Implement GetUserMedia device ID constraint processing in the renderer. (Closed)
|
Created:
4 years ago by Guido Urdaneta Modified:
4 years ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the processing of the deviceId constraint for getUserMedia in the renderer process.
This is a first step in the context of implementing MediaStreamTrack.applyConstraints.
In the original version, the renderer sends a structure containing zero or more candidate device IDs to the browser. The browser selected one candidate and tried to use that device if the requesting origin had authorization.
In the new version the renderer does the initial selection and sends zero or one candidate ID to the browser.
Note that in both cases the final decision about whether to use a candidate device ID requested by the renderer lies in the browser, which always checks that the requesting origin is authorized to use the corresponding device.
BUG=338503
Committed: https://crrev.com/a12310b23b600d971816c01bb5cd7f6034db50fa
Cr-Commit-Position: refs/heads/master@{#435896}
Patch Set 1 #Patch Set 2 : fixes #
Total comments: 14
Patch Set 3 : hta's comments #
Messages
Total messages: 34 (22 generated)
The CQ bit was checked by guidou@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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by guidou@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 ========== Move GetUserMedia device selection from browser to renderer. BUG=338503 ========== to ========== Move GetUserMedia device selection from browser to renderer. BUG=338503 ==========
guidou@chromium.org changed reviewers: + hta@chromium.org, tommi@chromium.org
guidou@chromium.org changed reviewers: + palmer@chromium.org
Hi, PTAL palmer@chromium.org: please review IPC content/common/media/media_stream_messages.h
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Mostly about clarity of comments. I do worry about the fact that we still pick a device, and then check it against constraints, instead of inspecting constraints against the device list to figure out which one we should ask for. But that's a later step. https://codereview.chromium.org/2538033003/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2538033003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:635: // Test that we can generate streams where a optional sourceId is specified in Is this sourceid still optional? Please update comment. https://codereview.chromium.org/2538033003/diff/20001/content/common/media/me... File content/common/media/media_stream_options.h (right): https://codereview.chromium.org/2538033003/diff/20001/content/common/media/me... content/common/media/media_stream_options.h:37: // Nonempty string represents a specific device. Nit, English: "An empty string represents the default device. A nonempty string represents a specific device." https://codereview.chromium.org/2538033003/diff/20001/content/common/media/me... content/common/media/media_stream_options.h:42: // to the renderer process in order to control the opening of a device Fix the grammar of "to the browser proces to the renderer process", please - I think the second "to" should be "from". https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:74: // and true is returned. Suggested reword: If a deviceId requested as mandatory is found in |device_infos|, <something happens>. If it is not found, the function returns false. If a valid device ID is found in ideal or advanced constraints, it is returned in |*device_id|. If no valid device ID is found, |*device_id| is left unmodified and true is returned. Note that this will pick *any* device, whether or not it is able to satisfy the other constraints, and whether or not it is the best one. You may want to add a TODO here. https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:147: if (audio_constraints.basic().hotwordEnabled.hasExact()) { This looks like you should be using the constraint pickers from media_stream_constraints_util.h. Consistent behavior is important. https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:247: controls->video.requested = true; I added the ability to initialize an UserMediaRequest to a sensible value for testing some time back, but never got around to changing this set of tests to use this feature. "We are in a test" code is a Bad Idea; if you can get rid of it, please do. https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:283: GetMediaDevicesDispatcher()->EnumerateDevices( Will this use a cache if it's available?
Neither the bug nor the CL description explains what's going on, and I don't see a design document. It *sounds* like it means that the renderer will control what devices the web content gets to use. Is that right? If so, it would be a security issue: renderers are not trustworthy. We can only trust the browser process to make a decision that important. If you want to have the render *advise* the browser about what devices to suggest the user choose from, that's fine. But the CL description doesn't make that clear.
Hm. That's a new requirement, if so. The renderer mediates the user's desire for devices. I do not know offhand where the code for permission prompts lives, but it is not touched by this cl. 30. nov. 2016 21:38 skrev <palmer@chromium.org>: Neither the bug nor the CL description explains what's going on, and I don't see a design document. It *sounds* like it means that the renderer will control what devices the web content gets to use. Is that right? If so, it would be a security issue: renderers are not trustworthy. We can only trust the browser process to make a decision that important. If you want to have the render *advise* the browser about what devices to suggest the user choose from, that's fine. But the CL description doesn't make that clear. https://codereview.chromium.org/2538033003/ -- 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.
On 2016/11/30 20:38:36, palmer wrote: > Neither the bug nor the CL description explains what's going on, and I don't see > a design document. > The bug is about implementing MediaStreamTrack.applyConstraints. This CL does not fix the bug, but it is a preliminary CL in that context of that bug, which will require several CLs. We have several preliminary documents about the new version of constraints processing, but before proceeding further we want to simplify the structure of the current (non-spec compliant) constraint processing. This is a first step in that direction (will update description to reflect that). > It *sounds* like it means that the renderer will control what devices the web > content gets to use. Is that right? If so, it would be a security issue: > renderers are not trustworthy. We can only trust the browser process to make a > decision that important. > > If you want to have the render *advise* the browser about what devices to > suggest the user choose from, that's fine. But the CL description doesn't make > that clear. The way this currently works is that the web application provides a list of possible devices to use (using obfuscated device IDs, since the real ones are never available to the renderer). The selection of which device to use follows an algorithm implemented (before this CL) in MediaStreamManager::PickDeviceId. This CL moves that algorithm to the renderer, in UserMediaClientImpl::PickDeviceId. I don't see any extra security issue in this CL since a rogue renderer can request any device ID it wants to the browser via IPC in both cases. The browser will use the device requested by the renderer only if the user has given permission to the requesting origin. This CL does not change that in any way. The only difference, IPC wise, is that the renderer now may only send zero or one device ID to the browser, whereas in the past it could send zero, one or many IDs.
Description was changed from ========== Move GetUserMedia device selection from browser to renderer. BUG=338503 ========== to ========== Move the device-selection algorithm for getUserMedia from the browser to renderer. This is a first step in the context of implementing MediaStreamTrack.applyConstraints. Note that the ultimate decision about whether to use a device requested by the renderer lies in the browser, which always checks that the requesting origin is authorized to use that device. BUG=338503 ==========
I think the description can be improved. As is, it's easy to read it in the way palmer@ did and I as well. Code lgtm.
Description was changed from ========== Move the device-selection algorithm for getUserMedia from the browser to renderer. This is a first step in the context of implementing MediaStreamTrack.applyConstraints. Note that the ultimate decision about whether to use a device requested by the renderer lies in the browser, which always checks that the requesting origin is authorized to use that device. BUG=338503 ========== to ========== Implement the processing of the deviceId constraint for getUserMedia in the renderer process. This is a first step in the context of implementing MediaStreamTrack.applyConstraints. In the original version, the renderer sends a structure containing zero or more candidate device IDs to the browser. The browser selected one candidate and tried to use that device if the requesting origin had authorization. In the new version the renderer does the initial selection and sends zero or one candidate ID to the browser. Note that in both cases the final decision about whether to use a candidate device ID requested by the renderer lies in the browser, which always checks that the requesting origin is authorized to use the corresponding device. BUG=338503 ==========
https://codereview.chromium.org/2538033003/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2538033003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:635: // Test that we can generate streams where a optional sourceId is specified in On 2016/11/30 18:51:42, hta - Chromium wrote: > Is this sourceid still optional? Please update comment. Actually, this test was redundant in the original code. It was exactly the same as the test formerly called GenerateStreamsWithMandatorySourceId. Removed. https://codereview.chromium.org/2538033003/diff/20001/content/common/media/me... File content/common/media/media_stream_options.h (right): https://codereview.chromium.org/2538033003/diff/20001/content/common/media/me... content/common/media/media_stream_options.h:37: // Nonempty string represents a specific device. On 2016/11/30 18:51:42, hta - Chromium wrote: > Nit, English: "An empty string represents the default device. A nonempty string > represents a specific device." Done. https://codereview.chromium.org/2538033003/diff/20001/content/common/media/me... content/common/media/media_stream_options.h:42: // to the renderer process in order to control the opening of a device On 2016/11/30 18:51:43, hta - Chromium wrote: > Fix the grammar of "to the browser proces to the renderer process", please - I > think the second "to" should be "from". Done. https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:74: // and true is returned. On 2016/11/30 18:51:43, hta - Chromium wrote: > Suggested reword: > > If a deviceId requested as mandatory is found in |device_infos|, <something > happens>. > If it is not found, the function returns false. > If a valid device ID is found in ideal or advanced constraints, it is returned > in |*device_id|. > If no valid device ID is found, |*device_id| is left unmodified and true is > returned. Done. > Note that this will pick *any* device, whether or not it is able to satisfy the > other constraints, and whether or not it is the best one. > You may want to add a TODO here. Done. At the moment this just replicates the existing logic in the browser. https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:147: if (audio_constraints.basic().hotwordEnabled.hasExact()) { On 2016/11/30 18:51:43, hta - Chromium wrote: > This looks like you should be using the constraint pickers from > media_stream_constraints_util.h. Consistent behavior is important. > > This part is copied practically verbatim from the former CopyBlinkRequestToStreamControls. Note that the processing is different for hotword and disableLocalEcho, so a generic function that works for both is probably not practical. https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:247: controls->video.requested = true; On 2016/11/30 18:51:43, hta - Chromium wrote: > I added the ability to initialize an UserMediaRequest to a sensible value for > testing some time back, but never got around to changing this set of tests to > use this feature. > > "We are in a test" code is a Bad Idea; if you can get rid of it, please do. Added TODO. Will try to do that when implementing the new processing algorithm. https://codereview.chromium.org/2538033003/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:283: GetMediaDevicesDispatcher()->EnumerateDevices( On 2016/11/30 18:51:43, hta - Chromium wrote: > Will this use a cache if it's available? Yes, it will. At the moment the cache is on the browser side. We will also introduce a renderer-side cache when we implement the latest version of the devicechange event, but that should be largely transparent to this part of the code.
palmer@: The description has been updated to better reflect what this CL does. Can you take another look?
The CQ bit was checked by guidou@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.
lgtm
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2538033003/#ps40001 (title: "hta's comments")
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": 40001, "attempt_start_ts": 1480670551811240,
"parent_rev": "69520e245db98ef4a719e20018ce29ea6844fb1c", "commit_rev":
"2ef34e932805692e204ccdb4eaac6842bfb6eced"}
Message was sent while issue was closed.
Description was changed from ========== Implement the processing of the deviceId constraint for getUserMedia in the renderer process. This is a first step in the context of implementing MediaStreamTrack.applyConstraints. In the original version, the renderer sends a structure containing zero or more candidate device IDs to the browser. The browser selected one candidate and tried to use that device if the requesting origin had authorization. In the new version the renderer does the initial selection and sends zero or one candidate ID to the browser. Note that in both cases the final decision about whether to use a candidate device ID requested by the renderer lies in the browser, which always checks that the requesting origin is authorized to use the corresponding device. BUG=338503 ========== to ========== Implement the processing of the deviceId constraint for getUserMedia in the renderer process. This is a first step in the context of implementing MediaStreamTrack.applyConstraints. In the original version, the renderer sends a structure containing zero or more candidate device IDs to the browser. The browser selected one candidate and tried to use that device if the requesting origin had authorization. In the new version the renderer does the initial selection and sends zero or one candidate ID to the browser. Note that in both cases the final decision about whether to use a candidate device ID requested by the renderer lies in the browser, which always checks that the requesting origin is authorized to use the corresponding device. BUG=338503 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Implement the processing of the deviceId constraint for getUserMedia in the renderer process. This is a first step in the context of implementing MediaStreamTrack.applyConstraints. In the original version, the renderer sends a structure containing zero or more candidate device IDs to the browser. The browser selected one candidate and tried to use that device if the requesting origin had authorization. In the new version the renderer does the initial selection and sends zero or one candidate ID to the browser. Note that in both cases the final decision about whether to use a candidate device ID requested by the renderer lies in the browser, which always checks that the requesting origin is authorized to use the corresponding device. BUG=338503 ========== to ========== Implement the processing of the deviceId constraint for getUserMedia in the renderer process. This is a first step in the context of implementing MediaStreamTrack.applyConstraints. In the original version, the renderer sends a structure containing zero or more candidate device IDs to the browser. The browser selected one candidate and tried to use that device if the requesting origin had authorization. In the new version the renderer does the initial selection and sends zero or one candidate ID to the browser. Note that in both cases the final decision about whether to use a candidate device ID requested by the renderer lies in the browser, which always checks that the requesting origin is authorized to use the corresponding device. BUG=338503 Committed: https://crrev.com/a12310b23b600d971816c01bb5cd7f6034db50fa Cr-Commit-Position: refs/heads/master@{#435896} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a12310b23b600d971816c01bb5cd7f6034db50fa Cr-Commit-Position: refs/heads/master@{#435896} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
