Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12440027:
Do not pass the string device_id via IPC message to create an audio input stream (Closed)
|
Created:
7 years, 9 months ago by no longer working on chromium Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, DaleCurtis Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDo not pass the string device_id via IPC message to create an audio input stream.
Using a string device_id via IPC message from the render client to open the device is not very safe since IPC message is not trusted. Instead, we should just pass an int (session_id) to the browser, and we do the lookup on AudioInputDeviceManager in the browser to re-map the session_id to the device_id.
BUG=179597, 216952
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189342
Patch Set 1 #Patch Set 2 : ready for review #
Total comments: 18
Patch Set 3 : addressed palmer's comments #
Total comments: 6
Patch Set 4 : fixed the nits. #
Total comments: 9
Patch Set 5 : changed the pepper code to use OpenDevice() for default device, and fixed a minor mistake in MediaS… #
Total comments: 8
Patch Set 6 : addressed miu's comments. #Patch Set 7 : fixed the tab capture apitests #
Total comments: 6
Patch Set 8 : addressed Per's comments. #Messages
Total messages: 34 (0 generated)
Hi, this patch is mainly removing code and fixed the security concern from the device_id in the IPC msg. Please take a look if you have time. I will send out a formal review request tomorrow after going through the changes again tomorrow to make sure I do not miss anything. BR, SX
On 2013/03/11 21:57:10, xians1 wrote: > Hi, this patch is mainly removing code and fixed the security concern from the > device_id in the IPC msg. > > Please take a look if you have time. > I will send out a formal review request tomorrow after going through the changes > again tomorrow to make sure I do not miss anything. > > > BR, > SX Thanks for being proactive about this! :) The approach looks good, and it's great to see tons of LOC deleted and only a few added. Let me know when you want me to start formal review. Thanks, Yuri
Hi Reviewers, this CL is mostly cleaning up and removing code, and it also fixes the security concern from the device_id in the IPC msg. please review. Chris, we are using the session_id to tell the browser which device to use. If you want to do the similar things, we need to change media_stream_impl.cc code from webkit_sources[i].setDeviceId() to webkit_sources[i].setSessionId(). Please let me know if you have any question. Thanks, BR, SX
On 2013/03/12 10:43:25, xians1 wrote: > Hi Reviewers, this CL is mostly cleaning up and removing code, and it also fixes > the security concern from the device_id in the IPC msg. please review. > > Chris, we are using the session_id to tell the browser which device to use. If > you want to do the similar things, we need to change media_stream_impl.cc code > from webkit_sources[i].setDeviceId() to webkit_sources[i].setSessionId(). > Please let me know if you have any question. > > Thanks, > > BR, > SX Shijing, it is *very* painful to go changing the WebKit API now to not use a string, and chromium is potentially not the only client of the API in WebKit. Instead, it should be fairly easy to create a string such as "device0", "device1", "device2", based on the integer value. For this to work, we need to have a way to determine if the integer value is the default device, thus there must be a default integer value. It seems like 0 would be the logical candidate. Please make sure to create a formatted string as I recommend above, and verify that we indeed have a way to determine the "default device" from the integer value.
Thinking more about this, it doesn't seem like the right thing to do is use the sessionID because we need to be able to identify one or more input devices for a given session. Thus we need to pass a separate integer value representing the device. We definitely need a way to tell if the value is the default device.
Thanks for doing this! It's not every day you get to remove more lines than you add. Happiness. crogers: The original API was of somewhat dubious design, and changing it is an improvement. Furthermore, the new design does not require huge changes for any (hypothetical?) non-Chromium callers, and in fact might make their live a bit easier — the new design is smaller and simpler. This CL makes the IPC much easier to make safe and keep safe. There are also (admittedly marginal) performance gains (smaller IPCs, less memory copying, and the potential to use a vector instead of a map). In general, string-using IPCs are a bad anti-pattern, and require "lexical type checking" (i.e. disturbing hackery involving grovelling around in the strings) to make them safe. https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/audio_input_device_manager.cc:40: StreamDeviceMap::iterator device = devices_.find(session_id); Now that we are indexing this map with an int, perhaps it can become a vector (time and space savings)? And therefore, perhaps the int should be unsigned (and possibly also small, like uint16?)? Are session_ids ever negative by design? And, if you use a vector, you should use a range-checked accessor (at vs. operator[] — but double-check that it works), and/or do your own range-checking. We wouldn't want to index a vector with an index from an untrustworthy source without checking it first. Even with the checking it's still faster and smaller than a map. https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/audio_input_device_manager.cc:180: if (!devices_.insert(std::make_pair(session_id, info)).second) { I don't understand what the intention of this expression is. And won't line 182 just insert the pair again? https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/audio_input_renderer_host.cc:213: if (session_id != AudioInputDeviceManager::kFakeOpenSessionId) { NIT: Two spaces after the "!=". Just use one. https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/audio_input_renderer_host.cc:217: // An empty device info indicates that no permission has been granted . NIT: Space before period. https://codereview.chromium.org/12440027/diff/4001/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12440027/diff/4001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:46: // provided audio parameters, |session_id| is used to be passed to the NIT: Extra space again. :) https://codereview.chromium.org/12440027/diff/4001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:143: int session_id_; As discussed in other comments, it *might* make sense for this to be unsigned and possibly also less wide. https://codereview.chromium.org/12440027/diff/4001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/4001/content/renderer/pepper/pe... content/renderer/pepper/pepper_platform_audio_input_impl.cc:151: // TODO(xians): this bypasses the permission and create a stream using the Is that safe? (Doesn't sound safe.) Is this change needed for the refactoring CL? https://codereview.chromium.org/12440027/diff/4001/media/base/audio_capturer_... File media/base/audio_capturer_source.h (right): https://codereview.chromium.org/12440027/diff/4001/media/base/audio_capturer_... media/base/audio_capturer_source.h:41: // be used. For clients who does not care about device permission and device Typo: does --> do https://codereview.chromium.org/12440027/diff/4001/media/base/audio_capturer_... media/base/audio_capturer_source.h:42: // selection, pass |session_id| using a value of 1 Just document the named constant, not the magic number 1. Callers should not rely on magic numbers.
Hi Chris, We are using AudioInputDeviceManager::kFakeOpenSessionId (which is 1) for clients to open the stream using the default device without permission checking. But I guess it is not sufficient for what you want in case you want to know if a valid session is using default device or not. I believe we can pass both the device_id and session_id to webkit, and you rechieve them back to webaudio in chromium, and verify if the device is a default device, but use the session_id to open the stream. On Tue, Mar 12, 2013 at 6:37 PM, <crogers@google.com> wrote: > Thinking more about this, it doesn't seem like the right thing to do is > use the > sessionID because we need to be able to identify one or more input devices > for a > given session. I am not fully following here, could you please elaborate a bit? The session_id in media stream means a media session and mapping to only one device. But I do remember some discussions that a session can ask for permission for multiple devices and decide one to use, but I am afraid that sessionID is not the same the session_id that we are using. > Thus we need to pass a separate integer value representing the > device. We definitely need a way to tell if the value is the default > device. > Is it fine to pass a string as today? then we only need to pass one more int (session_id). > https://codereview.chromium.**org/12440027/<https://codereview.chromium.org/1... >
Shijing, we just need a unique identifier (one for each device) which is an integer where we can check its value in the renderer. I know we have a list of input devices in the browser, so this value could be as simple as the index into this list. This integer value should be essentially an index representing one of all the available input devices we have because the Web platform APIs allow for handling multiple input devices. This integer value should be checkable to see if it represents the "default" device (probably can just make value 0 be default). Don't worry about the string part. Once we have such an integer as I describe above, then a string can be formatted such as 0 -> "device0", 1 -> "device1". That's the easy part. When AudioOutputDevice (AudioDevice) becomes more powerful then we can pass this deviceID (as integer) to select which one. We should make no assumptions that there will be only one input device per "session".
Hi reviewers, please take another look. SX https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/audio_input_device_manager.cc:40: StreamDeviceMap::iterator device = devices_.find(session_id); On 2013/03/12 17:45:11, Chris P. wrote: > Now that we are indexing this map with an int, perhaps it can become a vector > (time and space savings)? > > And therefore, perhaps the int should be unsigned (and possibly also small, like > uint16?)? Are session_ids ever negative by design? > > And, if you use a vector, you should use a range-checked accessor (at vs. > operator[] — but double-check that it works), and/or do your own range-checking. > We wouldn't want to index a vector with an index from an untrustworthy source > without checking it first. Even with the checking it's still faster and smaller > than a map. Done with changing to a vector. Now we use -1 to initialize the session_id to an invalid value. session_id was designed to be int for uniformity and is used in all the media stream code and its webkit code. It is very hard to change it to other type in one place but not others. I will suggest keeping it for now and do it in another refactoring CL if we decide to move to unsigned. https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/audio_input_device_manager.cc:180: if (!devices_.insert(std::make_pair(session_id, info)).second) { On 2013/03/12 17:45:11, Chris P. wrote: > I don't understand what the intention of this expression is. > > And won't line 182 just insert the pair again? This code has been removed. insert() will fail and never insert the item when the same session_id has been in the map, this should not happen so we put a NOTREACHED() in line 181. But the new device should overwrite the old one, that is why line 182 inserts the pair again. https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/audio_input_renderer_host.cc:213: if (session_id != AudioInputDeviceManager::kFakeOpenSessionId) { On 2013/03/12 17:45:11, Chris P. wrote: > NIT: Two spaces after the "!=". Just use one. Done. https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/audio_input_renderer_host.cc:217: // An empty device info indicates that no permission has been granted . On 2013/03/12 17:45:11, Chris P. wrote: > NIT: Space before period. Done. https://codereview.chromium.org/12440027/diff/4001/content/renderer/media/web... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12440027/diff/4001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:46: // provided audio parameters, |session_id| is used to be passed to the On 2013/03/12 17:45:11, Chris P. wrote: > NIT: Extra space again. :) Done. https://codereview.chromium.org/12440027/diff/4001/content/renderer/media/web... content/renderer/media/webrtc_audio_capturer.h:143: int session_id_; On 2013/03/12 17:45:11, Chris P. wrote: > As discussed in other comments, it *might* make sense for this to be unsigned > and possibly also less wide. Answered in that comment, anyhow this should be done on a separate CL and all the session_id should be changed to one type. https://codereview.chromium.org/12440027/diff/4001/content/renderer/pepper/pe... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/4001/content/renderer/pepper/pe... content/renderer/pepper/pepper_platform_audio_input_impl.cc:151: // TODO(xians): this bypasses the permission and create a stream using the On 2013/03/12 17:45:11, Chris P. wrote: > Is that safe? (Doesn't sound safe.) Is this change needed for the refactoring > CL? We allow pepper flash to use the default device without permission checking, it is because pepper is using another way for permission and I am afraid we can't change it now. https://codereview.chromium.org/12440027/diff/4001/media/base/audio_capturer_... File media/base/audio_capturer_source.h (right): https://codereview.chromium.org/12440027/diff/4001/media/base/audio_capturer_... media/base/audio_capturer_source.h:41: // be used. For clients who does not care about device permission and device On 2013/03/12 17:45:11, Chris P. wrote: > Typo: does --> do Done. https://codereview.chromium.org/12440027/diff/4001/media/base/audio_capturer_... media/base/audio_capturer_source.h:42: // selection, pass |session_id| using a value of 1 On 2013/03/12 17:45:11, Chris P. wrote: > Just document the named constant, not the magic number 1. Callers should not > rely on magic numbers. Done.
https://codereview.chromium.org/12440027/diff/15028/content/browser/renderer_... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/12440027/diff/15028/content/browser/renderer_... content/browser/renderer_host/media/audio_input_device_manager.cc:200: for (StreamDeviceList::iterator i(devices_.begin()); i != devices_.end(); Why do a linear search through a vector? Why not just index it directly (after checking that i > 0 < devices_.size(), of course)? https://codereview.chromium.org/12440027/diff/15028/content/renderer/media/we... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12440027/diff/15028/content/renderer/media/we... content/renderer/media/webrtc_audio_capturer.cc:223: DCHECK_GT(session_id_, 0); Perhaps this should be a run-time check. https://codereview.chromium.org/12440027/diff/15028/content/renderer/pepper/p... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/15028/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:154: // TODO(xians): this bypasses the permission and create a stream using the NIT: Grammar and punctuation. "This bypasses... and creates... . We should remove..."
moving myself to cc, this all looks good to me, but Yuri and Chris are more familiar with the input side of things. One obvious thing I'll point out here is that session ids are sequential, so there are still opportunities for "attacks" which access another session id. Given that we define permission as meaning access to any input device, this is probably fine.
Hi Shijing, Just a friendly notice: Due to scheduling conflicts on my end, I'll probably be looking at this late tomorrow, or more likely on Monday.
Yuri, it is fine for you to do it when you have time. Palmer, another look? Yzshen, owner stamp for the pepper code, and any comment about removing pepper_platform_audio_input_impl.cc line 153-160? Jam, owner stamp for the content_browser.gypi. Thanks https://codereview.chromium.org/12440027/diff/15028/content/browser/renderer_... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/12440027/diff/15028/content/browser/renderer_... content/browser/renderer_host/media/audio_input_device_manager.cc:200: for (StreamDeviceList::iterator i(devices_.begin()); i != devices_.end(); On 2013/03/14 21:05:14, Chris P. wrote: > Why do a linear search through a vector? Why not just index it directly (after > checking that i > 0 < devices_.size(), of course)? As discussed offline, the session_id_ can't be used for indexing. https://codereview.chromium.org/12440027/diff/15028/content/renderer/media/we... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/12440027/diff/15028/content/renderer/media/we... content/renderer/media/webrtc_audio_capturer.cc:223: DCHECK_GT(session_id_, 0); On 2013/03/14 21:05:14, Chris P. wrote: > Perhaps this should be a run-time check. sgtm, making it a CHECK https://codereview.chromium.org/12440027/diff/15028/content/renderer/pepper/p... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/15028/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:154: // TODO(xians): this bypasses the permission and create a stream using the On 2013/03/14 21:05:14, Chris P. wrote: > NIT: Grammar and punctuation. > > "This bypasses... and creates... . We should remove..." Done.
On 2013/03/15 14:52:11, xians1 wrote: > Yuri, it is fine for you to do it when you have time. > Palmer, another look? > > Yzshen, owner stamp for the pepper code, and any comment about removing > pepper_platform_audio_input_impl.cc line 153-160? > > Jam, owner stamp for the content_browser.gypi. lgtm
IPC security LGTM
https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). What do you mean by "Pepper completely switches to OpenDevice()"? The current PPB_AudioInput_Dev.Open() API can be instructed to open the default device which will take this code path. Do you mean we need to change it? https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:184: ipc_->CreateStream(stream_id_, session_id, params_, false, 1); This ignores |session_id| if it comes from OnDeviceOpened(). Will it break device selection?
https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). On 2013/03/15 18:21:03, yzshen1 wrote: > What do you mean by "Pepper completely switches to OpenDevice()"? The current > PPB_AudioInput_Dev.Open() API can be instructed to open the default device which > will take this code path. Do you mean we need to change it? We don't want clients to be able to open the default device without getting the permission. That means it is NOTREACHED if (device_id.empty()). If Pepper wants to create an input stream, it has to go through the OpenDevice path https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:184: ipc_->CreateStream(stream_id_, session_id, params_, false, 1); On 2013/03/15 18:21:03, yzshen1 wrote: > This ignores |session_id| if it comes from OnDeviceOpened(). Will it break > device selection? We don't ignore the session_id, it is just moved to the second param, and the 1 (last param) is the number of share memory.
LGTM Thanks! https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). Just out of curiosity: Do we have a way to specify "default device" when going through the OpenDevice path? On 2013/03/15 19:16:48, xians1 wrote: > On 2013/03/15 18:21:03, yzshen1 wrote: > > What do you mean by "Pepper completely switches to OpenDevice()"? The current > > PPB_AudioInput_Dev.Open() API can be instructed to open the default device > which > > will take this code path. Do you mean we need to change it? > > We don't want clients to be able to open the default device without getting the > permission. That means it is NOTREACHED if (device_id.empty()). > If Pepper wants to create an input stream, it has to go through the OpenDevice > path https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:184: ipc_->CreateStream(stream_id_, session_id, params_, false, 1); I see. :) Sorry for not reading carefully enough. On 2013/03/15 19:16:48, xians1 wrote: > On 2013/03/15 18:21:03, yzshen1 wrote: > > This ignores |session_id| if it comes from OnDeviceOpened(). Will it break > > device selection? > > We don't ignore the session_id, it is just moved to the second param, and the 1 > (last param) is the number of share memory.
https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). On 2013/03/15 21:47:48, yzshen1 wrote: > Just out of curiosity: Do we have a way to specify "default device" when going > through the OpenDevice path? > Yes, just pass AudioManagerBase::kDefaultDeviceId as the input to OpenDevice. Do you want me to change the code to use it in this CL? > On 2013/03/15 19:16:48, xians1 wrote: > > On 2013/03/15 18:21:03, yzshen1 wrote: > > > What do you mean by "Pepper completely switches to OpenDevice()"? The > current > > > PPB_AudioInput_Dev.Open() API can be instructed to open the default device > > which > > > will take this code path. Do you mean we need to change it? > > > > We don't want clients to be able to open the default device without getting > the > > permission. That means it is NOTREACHED if (device_id.empty()). > > If Pepper wants to create an input stream, it has to go through the OpenDevice > > path > https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:184: ipc_->CreateStream(stream_id_, session_id, params_, false, 1); On 2013/03/15 21:47:48, yzshen1 wrote: > I see. :) Sorry for not reading carefully enough. > No problem. > On 2013/03/15 19:16:48, xians1 wrote: > > On 2013/03/15 18:21:03, yzshen1 wrote: > > > This ignores |session_id| if it comes from OnDeviceOpened(). Will it break > > > device selection? > > > > We don't ignore the session_id, it is just moved to the second param, and the > 1 > > (last param) is the number of share memory. >
https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/p... content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). Either way. It sounds good if you change it directly, though. :) On 2013/03/15 21:57:43, xians1 wrote: > On 2013/03/15 21:47:48, yzshen1 wrote: > > Just out of curiosity: Do we have a way to specify "default device" when going > > through the OpenDevice path? > > > > Yes, just pass AudioManagerBase::kDefaultDeviceId as the input to OpenDevice. > > Do you want me to change the code to use it in this CL? > > > > On 2013/03/15 19:16:48, xians1 wrote: > > > On 2013/03/15 18:21:03, yzshen1 wrote: > > > > What do you mean by "Pepper completely switches to OpenDevice()"? The > > current > > > > PPB_AudioInput_Dev.Open() API can be instructed to open the default device > > > which > > > > will take this code path. Do you mean we need to change it? > > > > > > We don't want clients to be able to open the default device without getting > > the > > > permission. That means it is NOTREACHED if (device_id.empty()). > > > If Pepper wants to create an input stream, it has to go through the > OpenDevice > > > path > > >
yzshen, FYI, I have already changed the code to use OpenDevice when the device_id.empty() is true. croger, I tested the CL with http://chromium.googlecode.com/svn/trunk/samples/audio/visualizer-live.html, it works well with the default device. miu, please review it when you have time. SX
Thanks! Still LGTM
lgtm Great work! It's very nice to see a ton of complexity removed and simplified. Please address nits before committing... https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... File content/browser/renderer_host/media/audio_input_device_manager.h (right): https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... content/browser/renderer_host/media/audio_input_device_manager.h:43: StreamDeviceInfo GetOpenedDeviceInfoById(int session_id); nit: Consider returning a bool and having the caller provide a pointer to a StreamDeviceInfo (output argument). Seems less error-prone than looking at StreamDeviceInfo and trying to guess whether its uninitialized. https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... content/browser/renderer_host/media/audio_input_device_manager.h:81: StreamDeviceList::iterator GetDevice(int session_id); Suggest you return a StreamDeviceInfo* instead of an iterator here. Return NULL to mean not found, etc. https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... File content/browser/renderer_host/media/audio_input_renderer_host.h (right): https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... content/browser/renderer_host/media/audio_input_renderer_host.h:91: void OnCreateStream(int stream_id, nit: Please add comment for this method: what session_id is and that it's optional. https://codereview.chromium.org/12440027/diff/37001/content/renderer/media/we... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12440027/diff/37001/content/renderer/media/we... content/renderer/media/webrtc_audio_capturer.h:46: // provided audio parameters, |session_id| is used to be passed to the nit: /is used to be passed/is passed/
https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... File content/browser/renderer_host/media/audio_input_device_manager.h (right): https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... content/browser/renderer_host/media/audio_input_device_manager.h:43: StreamDeviceInfo GetOpenedDeviceInfoById(int session_id); On 2013/03/18 23:19:05, Yuri wrote: > nit: Consider returning a bool and having the caller provide a pointer to a > StreamDeviceInfo (output argument). Seems less error-prone than looking at > StreamDeviceInfo and trying to guess whether its uninitialized. Then I think it might be clearer to return a const StreamDeviceInfo* for this GetOpenedDeviceInfoById(). Let me know if you disagree, I can upload a new CL to change it to accept a pointer for the output. https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... content/browser/renderer_host/media/audio_input_device_manager.h:81: StreamDeviceList::iterator GetDevice(int session_id); On 2013/03/18 23:19:05, Yuri wrote: > Suggest you return a StreamDeviceInfo* instead of an iterator here. Return NULL > to mean not found, etc. This iterator returned by GetDevice() is used by Close() to delete the item in the vector. Returning a StreamDeviceInfo* means we need to loop through the vector again to find out what to delete. Hope it is fine to keep this way. https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... File content/browser/renderer_host/media/audio_input_renderer_host.h (right): https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_... content/browser/renderer_host/media/audio_input_renderer_host.h:91: void OnCreateStream(int stream_id, On 2013/03/18 23:19:05, Yuri wrote: > nit: Please add comment for this method: what session_id is and that it's > optional. Done with updating the comment. https://codereview.chromium.org/12440027/diff/37001/content/renderer/media/we... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/12440027/diff/37001/content/renderer/media/we... content/renderer/media/webrtc_audio_capturer.h:46: // provided audio parameters, |session_id| is used to be passed to the On 2013/03/18 23:19:05, Yuri wrote: > nit: /is used to be passed/is passed/ Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/46001
Presubmit check for 12440027-46001 failed and returned exit status 1.
INFO:root:Found 22 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
media/base/audio_capturer_source.h
Presubmit checks took 2.5s to calculate.
Dale, can I have your owner stamp for media/base/audio_capturer_source.h? Thanks, SX
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/46001
https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/me... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:498: MediaStreamExtraData* extra_data = static_cast<MediaStreamExtraData*>( Looks like all of this belongs in the same place as were you have the audio capturer - ie move to a function in dependency_factory_ that takes(*request_it)->descriptor as argument. https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:502: // We need to stop the capturer for local media stream. audio track - not stream https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:513: media_stream_dispatcher_->StopStream( So is StopStream only used for video now or audio as well?
Per, could you please take another look at the media_stream_impl and media_stream_dependency_factory? Thanks, SX https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/me... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:498: MediaStreamExtraData* extra_data = static_cast<MediaStreamExtraData*>( On 2013/03/20 10:39:53, perkj wrote: > Looks like all of this belongs in the same place as were you have the audio > capturer - ie move to a function in dependency_factory_ that > takes(*request_it)->descriptor as argument. Done. https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:502: // We need to stop the capturer for local media stream. On 2013/03/20 10:39:53, perkj wrote: > audio track - not stream Done. https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:513: media_stream_dispatcher_->StopStream( On 2013/03/20 10:39:53, perkj wrote: > So is StopStream only used for video now or audio as well? both audio and video as before.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/73002
MediastreamImpl and MediaStreamDependencyFactory lgtm.
Message was sent while issue was closed.
Change committed as 189342 |
