Adding MediaStreamManager.

Adding MediaStreamManager.

MediaStreamManager will get requests to open capture devices, normally from WebKit. MediaStreamManager will ask for user permission and then call the proper device manager to open the user specified device.

MediaStreamManager now owns VideoCaptureManager, which introduces changes to VideoCaptureManager and test related to VideoCaptureManager.

This patch will be followed by a patch to remove the singleton property.

The next MediaStream patch, MediaStreamDispatcherHost, will contain test including MediaStreamManager.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91696

[wjia(left Chromium), 2011-07-01 03:58:33]

took a quick look at design. did not look at code style.

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_device_settings.cc:50: void
MediaStreamDeviceSettings::RequestCaptureDeviceUsage(
Is it good to combine RequestCaptureDeviceUsage with AvailableDevices? From the
code, it looks that the same requester will provide device list later. Combining
those 2 function can simplify the interface between requester and
DeviceSettings, and save a round trip between them (GetDevice and
AvailableDevices).

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.cc:30: int random_char
= base::RandGenerator(sizeof(alphabet - 1));
would it be "sizeof(alphabet) - 1"?

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.h:79: virtual void
Error(const std::string& label);
it would be great if different names are used for these 2 Error(...).

http://codereview.chromium.org/7284037/diff/1/content/common/media/media_stre...
File content/common/media/media_stream_options.h (right):

http://codereview.chromium.org/7284037/diff/1/content/common/media/media_stre...
content/common/media/media_stream_options.h:42: const int kNumMediaStreamTypes =
3;
moving kNumMediaStreamTypes into enum can remove the hard-coded value.

[mflodman1, 2011-07-01 09:37:10]

Uploaded new patch based on review by wjia.

Thanks Wei!

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_device_settings.cc:50: void
MediaStreamDeviceSettings::RequestCaptureDeviceUsage(
On 2011/07/01 03:58:34, wjia wrote:
> Is it good to combine RequestCaptureDeviceUsage with AvailableDevices? From
the
> code, it looks that the same requester will provide device list later.
Combining
> those 2 function can simplify the interface between requester and
> DeviceSettings, and save a round trip between them (GetDevice and
> AvailableDevices).

This is a good point and I was thinking about this too. I made it this way since
I don't know exactly what the UI guys need and how they would like to use this.
This allows them to call GetDevices to populate a UI without getting a device
request, which is useful in case of having a settings page rather than a pop-up
only when starting a camera.

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.cc:30: int random_char
= base::RandGenerator(sizeof(alphabet - 1));
On 2011/07/01 03:58:34, wjia wrote:
> would it be "sizeof(alphabet) - 1"?

Yes, you're right. Otherwise we'll end up with only numbers in the label.

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/7284037/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.h:79: virtual void
Error(const std::string& label);
On 2011/07/01 03:58:34, wjia wrote:
> it would be great if different names are used for these 2 Error(...).

Agree, renamed to SettingsError. I'll add AudioInputManager shortly and I'll
wait with changing the name of the other error functinotil then, if you think
that's ok. Mainly since it will increase the size of this CL even more.

http://codereview.chromium.org/7284037/diff/1/content/common/media/media_stre...
File content/common/media/media_stream_options.h (right):

http://codereview.chromium.org/7284037/diff/1/content/common/media/media_stre...
content/common/media/media_stream_options.h:42: const int kNumMediaStreamTypes =
3;
On 2011/07/01 03:58:34, wjia wrote:
> moving kNumMediaStreamTypes into enum can remove the hard-coded value.

Done.

[Leandro Graciá Gil, 2011-07-01 11:31:35]

A few things to fix and consider. By the way, I'll be out until next Tuesday's
afternoon.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:24: static const
char alphabet[] = "0123456789"
I think this comes from the old demo code (and hence my error with the sizeof).
This should probably work, but I think we never checked that this alphabet was
exactly the one covered by the spec about the labels. Also, the length seems to
be outdated too. Could you please check it?

Copy/paste from today's version of the spec:
When a LocalMediaStream object is created, the user agent must generate a
globally unique identifier string, and must initialize the object's label
attribute to that string. Such strings must only use characters in the ranges
U+0021, U+0023 to U+0027, U+002A to U+002B, U+002D to U+002E, U+0030 to U+0039,
U+0041 to U+005A, U+005E to U+007E, and must be 36 characters long.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:40: &&
(options.video_option != StreamOptions::kNoCamera)) {
What if stream_type asks for video but options don't (or the other way around)?
This is being checked but it's probably something that shouldn't happen. Maybe
we should DCHECK it?

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:86: if
(new_request.label == it->label) {
Does the request list contain the labels of existing streams which requests have
been solved? I want to make sure that the random label doesn't collide with an
existing stream label (maybe even in the ended state, that's probably something
to discuss) and not only with ongoing requests.

In any case, have you considered using a separate set/hash_set for the labels in
use or even make the request list a map/hash_map indexed by its label? Probably
it's worth it to simplify the code, save some iterator loops and enhance the
asymptotic costs, but it's up to you. If you do, make sure no ordering issues
may arise when selecting the container.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:165: // The request
doesn't exist.
Can this happen in legitimate situations without being a failure? If not we
should DCHECK.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:169: device->in_use
= true;
Is there a possibility that the device is already in use? (I guess not, but I
see no reason not to ask.)

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:175: if
(request->state[kAudioCapture] == DeviceRequest::kOpening) {
Which other states are valid here? Maybe it's worth to DCHECK.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:210:
label_list.pop_front();
Since label_list is local and you always pop the first element in each
iteration, shouldn't it be easier and more efficient just to use a std::vector
for label_list and then iterate it?

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:228: DCHECK(false);
Use NOTREACHED().

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:246:
devices->erase(device_it);
This will move all the elements after it in the devices vector. If these erases
are going to be frequent maybe we should change the container type to something
else.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:303: DCHECK(false);
NOTREACHED()

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:168: const
size_t needed_size = (info.width * info.height * 3) / 2;
Just as a sidenote, I guess you know this better than me but make sure you don't
have any horizontal line alignment for whatever you're doing, or you will have
problems with some width values and alignment gaps.

http://codereview.chromium.org/7284037/diff/7001/content/common/media/media_s...
File content/common/media/media_stream_options.h (right):

http://codereview.chromium.org/7284037/diff/7001/content/common/media/media_s...
content/common/media/media_stream_options.h:38: kAudioCapture = 1,
Not a problem, but is there any reason of why you're setting all the values
here? Even if it is, it should be consistent with StreamOptions::VideoOption and
other enums in other files if possible.

Personally, I wouldn't set all the values, especially the counting one, but
that's up to you.

[mflodman1, 2011-07-01 14:07:22]

Uploaded new patch containing changes based on review by leandrogracia.

I have not changed DeviceRequestList to being a map yet, I'll have a look at
that but wanted to upload this right away.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:24: static const
char alphabet[] = "0123456789"
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> I think this comes from the old demo code (and hence my error with the
sizeof).
> This should probably work, but I think we never checked that this alphabet was
> exactly the one covered by the spec about the labels. Also, the length seems
to
> be outdated too. Could you please check it?
> 
> Copy/paste from today's version of the spec:
> When a LocalMediaStream object is created, the user agent must generate a
> globally unique identifier string, and must initialize the object's label
> attribute to that string. Such strings must only use characters in the ranges
> U+0021, U+0023 to U+0027, U+002A to U+002B, U+002D to U+002E, U+0030 to
U+0039,
> U+0041 to U+005A, U+005E to U+007E, and must be 36 characters long.

Length updated. The label is used in Chrome internally and is modified in some
way before it's delivered to the java script. I'll check with grunell about this
before changing the alphabet, putting a TODO for now.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:40: &&
(options.video_option != StreamOptions::kNoCamera)) {
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> What if stream_type asks for video but options don't (or the other way
around)?
> This is being checked but it's probably something that shouldn't happen. Maybe
> we should DCHECK it?
This is just a helper to see if the stream type is part of options or not.
(stream_type set by MediaStreamManager and options comes from WebKit). If it is
-> return true, else false. E.g. we want to return false if stream_type is
kVideoCapture and options.video_option == kNoCamera, i.e. we want to know if
video was requested but it wasn't.

Or did I misunderstand your question?

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:86: if
(new_request.label == it->label) {
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Does the request list contain the labels of existing streams which requests
have
> been solved? I want to make sure that the random label doesn't collide with an
> existing stream label (maybe even in the ended state, that's probably
something
> to discuss) and not only with ongoing requests.
> 

A request is stored until the stream is either closed by a call to
StopGeneratedStream or until the entire stream gets an error and
StreamGenerationFailed is signalled. So there should never be two streams using
the same label at any time.


> In any case, have you considered using a separate set/hash_set for the labels
in
> use or even make the request list a map/hash_map indexed by its label?
Probably
> it's worth it to simplify the code, save some iterator loops and enhance the
> asymptotic costs, but it's up to you. If you do, make sure no ordering issues
> may arise when selecting the container.

I'll look at this, but I'll upload a new patch with the other changes as a
start.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:165: // The request
doesn't exist.
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Can this happen in legitimate situations without being a failure? If not we
> should DCHECK.

Yes, the request can be canceled during the time the device is opened by
VideocaptureManager, which happens on another thread.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:169: device->in_use
= true;
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Is there a possibility that the device is already in use? (I guess not, but I
> see no reason not to ask.)

A device is marked as in_use by the VideoCaptureManager/AudioInputDeviceManager
if it has been opened in a stream already. For audio we allow the same device to
be opened several times, and might allow for video as well later, so for this
request we set in_use to false in DevicesAccepted before asking the
corresponding manager to open it. This is to be able to track when this request
is done.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:175: if
(request->state[kAudioCapture] == DeviceRequest::kOpening) {
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Which other states are valid here? Maybe it's worth to DCHECK.

It should only be kRequested that is invalid, meaning we have requested it from
the UI but never received user acceptance to use it. I'll add a DCHECK for that.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:210:
label_list.pop_front();
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Since label_list is local and you always pop the first element in each
> iteration, shouldn't it be easier and more efficient just to use a std::vector
> for label_list and then iterate it?

I changed to iterate the list and then do clear when everything is done instead
of the way it was. The drawback of using a vector is I don't know the size it
should be. I know how many requests there are in total and can allocate that
size of the vector, but there is a risk of over-allocating each time this is
done. Do you still think it's better using vector instead of list?

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:228: DCHECK(false);
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Use NOTREACHED().

Done.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:246:
devices->erase(device_it);
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> This will move all the elements after it in the devices vector. If these
erases
> are going to be frequent maybe we should change the container type to
something
> else.

It won't happen often and it's likely the vector contains only one element, so
I'll keep as is for now.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:303: DCHECK(false);
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> NOTREACHED()

Done.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_controller.cc:168: const
size_t needed_size = (info.width * info.height * 3) / 2;
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Just as a sidenote, I guess you know this better than me but make sure you
don't
> have any horizontal line alignment for whatever you're doing, or you will have
> problems with some width values and alignment gaps.

As it is now, this is an I420 frame without any alignment gaps, so it should be
fine here.

http://codereview.chromium.org/7284037/diff/7001/content/common/media/media_s...
File content/common/media/media_stream_options.h (right):

http://codereview.chromium.org/7284037/diff/7001/content/common/media/media_s...
content/common/media/media_stream_options.h:38: kAudioCapture = 1,
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Not a problem, but is there any reason of why you're setting all the values
> here? Even if it is, it should be consistent with StreamOptions::VideoOption
and
> other enums in other files if possible.
> 
> Personally, I wouldn't set all the values, especially the counting one, but
> that's up to you.

It was to explicitly show the number for, kNumMediaStreamTypes, which I agree is
not necessary and, in general, I do prefer the other convention over this.
Changed.

[mflodman1, 2011-07-04 21:43:14]

Updated DeviceRequestList to map and changes RandomLabel according to WhatWG
spec.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:24: static const
char alphabet[] = "0123456789"
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> I think this comes from the old demo code (and hence my error with the
sizeof).
> This should probably work, but I think we never checked that this alphabet was
> exactly the one covered by the spec about the labels. Also, the length seems
to
> be outdated too. Could you please check it?
> 
> Copy/paste from today's version of the spec:
> When a LocalMediaStream object is created, the user agent must generate a
> globally unique identifier string, and must initialize the object's label
> attribute to that string. Such strings must only use characters in the ranges
> U+0021, U+0023 to U+0027, U+002A to U+002B, U+002D to U+002E, U+0030 to
U+0039,
> U+0041 to U+005A, U+005E to U+007E, and must be 36 characters long.

Updated according to WhatWG spec.

http://codereview.chromium.org/7284037/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:86: if
(new_request.label == it->label) {
On 2011/07/01 11:31:35, Leandro Graciá Gil wrote:
> Does the request list contain the labels of existing streams which requests
have
> been solved? I want to make sure that the random label doesn't collide with an
> existing stream label (maybe even in the ended state, that's probably
something
> to discuss) and not only with ongoing requests.
> 
> In any case, have you considered using a separate set/hash_set for the labels
in
> use or even make the request list a map/hash_map indexed by its label?
Probably
> it's worth it to simplify the code, save some iterator loops and enhance the
> asymptotic costs, but it's up to you. If you do, make sure no ordering issues
> may arise when selecting the container.

Done.

[Leandro Graciá Gil, 2011-07-05 14:57:25]

LGTM.

[wjia(left Chromium), 2011-07-06 02:58:42]

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:44: &&
(options.video_option != StreamOptions::kNoCamera)) {
"&&" should be at the end of previous line.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:105: if
(it->second.requester == requester && !RequestDone(it->second)) {
looks like a request can be cancelled if any requested device is not opened
(i.e., in_use == false for either audio or video). What if that request has both
audio and video, and audio device is not opened but video device is opened? How
will that opened video device be closed?

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:107: it =
requests_.begin();
do you have to go from the beginning again? are you trying to erase an element
while iterating on a map?

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:212:
device_settings_->AvailableDevices(*it, stream_type, devices);
the label_list and 2nd for loop are not needed if this line is moved into first
for loop. Something like:

  for (DeviceRequests::iterator it = requests_.begin(); it != requests_.end();
         ++it) {
      if (it->second.state[stream_type] == DeviceRequest::kRequested
          && Requested(it->second.options, stream_type)) {
        device_settings_->AvailableDevices(it->first, stream_type, devices);
      }
    }

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:280: if (request_it
!= requests_.end()) {
is it possible only partial request is accepted? e.g., both audio and video are
requested, but only video is accepted.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:343: for
(StreamDeviceInfoArray::const_iterator it =
indentation.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.h:49:
VideoCaptureManager* GetVideoCaptureManager();
VideoCaptureManager* video_capture_manager();

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_settings_requester.h
(right):

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_settings_requester.h:10:
#include "base/task.h"
is this header file needed?

[mflodman1, 2011-07-06 11:31:45]

Uploaded changes based on review by Wei.

Good comments regarding the unusual cases, thanks!

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:44: &&
(options.video_option != StreamOptions::kNoCamera)) {
On 2011/07/06 02:58:42, wjia wrote:
> "&&" should be at the end of previous line.

Done.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:105: if
(it->second.requester == requester && !RequestDone(it->second)) {
On 2011/07/06 02:58:42, wjia wrote:
> looks like a request can be cancelled if any requested device is not opened
> (i.e., in_use == false for either audio or video). What if that request has
both
> audio and video, and audio device is not opened but video device is opened?
How
> will that opened video device be closed?

Very good point! I added code here to close open devices and  modified ::Opened
to set the state in a slightly different way.

Done.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:107: it =
requests_.begin();
On 2011/07/06 02:58:42, wjia wrote:
> do you have to go from the beginning again? are you trying to erase an element
> while iterating on a map?

You're right, there is no need to start from beginning.
Done.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:212:
device_settings_->AvailableDevices(*it, stream_type, devices);
On 2011/07/06 02:58:42, wjia wrote:
> the label_list and 2nd for loop are not needed if this line is moved into
first
> for loop. Something like:
> 
>   for (DeviceRequests::iterator it = requests_.begin(); it != requests_.end();
>          ++it) {
>       if (it->second.state[stream_type] == DeviceRequest::kRequested
>           && Requested(it->second.options, stream_type)) {
>         device_settings_->AvailableDevices(it->first, stream_type, devices);
>       }
>     }

I had it like that originally, but the problem was
device_settings_->AvailableDevices can trigger calls back to MediaStreamManager
that in an error case will modify requests_. This was a safe way of getting
around that.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:280: if (request_it
!= requests_.end()) {
On 2011/07/06 02:58:42, wjia wrote:
> is it possible only partial request is accepted? e.g., both audio and video
are
> requested, but only video is accepted.

Yes. No error is signalled to the requester if this happens, only the opened
devices, but I added an internal error state in MediaStreamManager to clarify.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:343: for
(StreamDeviceInfoArray::const_iterator it =
On 2011/07/06 02:58:42, wjia wrote:
> indentation.

Done.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.h:49:
VideoCaptureManager* GetVideoCaptureManager();
On 2011/07/06 02:58:42, wjia wrote:
> VideoCaptureManager* video_capture_manager();

Done.

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_settings_requester.h
(right):

http://codereview.chromium.org/7284037/diff/5002/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_settings_requester.h:10:
#include "base/task.h"
On 2011/07/06 02:58:42, wjia wrote:
> is this header file needed?

It was needd since DISABLE_RUNNABLE_METHOD_REFCOUNT was still at the bottom of
the file. However this is a left over from when this class was used when posting
task, which isn't done any longer. Removed.

[wjia(left Chromium), 2011-07-06 15:36:25]

lgtm. Since content/content_browser.gypi is touched, you might need content
owner's consent.

[commit-bot: I haz the power, 2011-07-06 19:51:07]

Try job failure for 7284037-18001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-07-07 13:05:23]

Change committed as 91696

[scherkus (not reviewing), 2011-07-08 20:56:11]

few tiny drive-by stuff.. mostly wondering about the singleton and tests

http://codereview.chromium.org/7284037/diff/19014/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7284037/diff/19014/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:21:
base::LazyInstance<MediaStreamManager> g_media_stream_manager(
drive by... can you address this soon?

this is one of those things that is easy to leave around (since it works) but
can be a source of startup/shutdown bugs

http://codereview.chromium.org/7284037/diff/19014/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:57: delete
device_settings_;
use scoped_ptr<> in .h

http://codereview.chromium.org/7284037/diff/19014/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:58: delete
video_capture_manager_;
use scoped_ptr<> in .h

http://codereview.chromium.org/7284037/diff/19014/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/7284037/diff/19014/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:41: class
MediaStreamManager : public MediaStreamProviderListener,
nit: should be initializer list style

http://codereview.chromium.org/7284037/diff/19014/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:45: static
MediaStreamManager* Get();
do we have existing tests that test this class or do they need to be written?