Do not pass the string device_id via IPC message to create an audio input stream

Do 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

[no longer working on chromium, 2013-03-11 21:57:10]

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

[miu, 2013-03-12 07:04:52]

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

[no longer working on chromium, 2013-03-12 10:43:25]

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

[Chris Rogers, 2013-03-12 17:17:44]

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.

[Chris Rogers, 2013-03-12 17:37:02]

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.

[palmer, 2013-03-12 17:45:11]

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.

[no longer working on chromium, 2013-03-12 17:58:31]

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...
>

[Chris Rogers, 2013-03-12 19:52:22]

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".

[no longer working on chromium, 2013-03-14 10:47:32]

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.

[palmer, 2013-03-14 21:05:14]

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..."

[DaleCurtis, 2013-03-14 21:53:20]

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.

[miu, 2013-03-15 09:50:26]

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.

[no longer working on chromium, 2013-03-15 14:52:11]

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.

[jam, 2013-03-15 16:11:12]

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

[palmer, 2013-03-15 18:08:14]

IPC security LGTM

[yzshen1, 2013-03-15 18:21:02]

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?

[no longer working on chromium, 2013-03-15 19:16:48]

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.

[yzshen1, 2013-03-15 21:47:47]

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.

[no longer working on chromium, 2013-03-15 21:57:43]

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.
>

[yzshen1, 2013-03-15 22:06:04]

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
> > 
>

[no longer working on chromium, 2013-03-18 14:18:29]

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

[yzshen1, 2013-03-18 16:55:31]

Thanks! Still LGTM

[miu, 2013-03-18 23:19:05]

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/

[no longer working on chromium, 2013-03-19 14:06:49]

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.

[commit-bot: I haz the power, 2013-03-19 14:07:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/46001

[commit-bot: I haz the power, 2013-03-19 14:07:27]

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.

[no longer working on chromium, 2013-03-19 14:32:09]

Dale, can I have your owner stamp for media/base/audio_capturer_source.h?

Thanks,
SX

[DaleCurtis, 2013-03-19 18:10:15]

lgtm

[commit-bot: I haz the power, 2013-03-19 18:21:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/46001

[perkj_chrome, 2013-03-20 10:39:52]

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?

[no longer working on chromium, 2013-03-20 15:10:19]

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.

[commit-bot: I haz the power, 2013-03-20 15:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/73002

[perkj_chrome, 2013-03-20 15:46:14]

MediastreamImpl and MediaStreamDependencyFactory lgtm.

[commit-bot: I haz the power, 2013-03-20 18:01:52]

Change committed as 189342