Extract VideoCaptureDeviceFactory out of VideoCaptureDevice and use for File and FakeVCD.

Extract VideoCaptureDeviceFactory out of VideoCaptureDevice and use for File and FakeVCD.

VideoCaptureDeviceFactory is owned by VideoCaptureManager.

VideoCaptureManager gets a VCDF on construction. This
can be a real devices factory or a test one (Fake/File).
All VCM operations previously static are now made
through the Factory API regardless of the type.

The previous MSM::UseFakeDevice() disappears and
unittests MediaStreamDispatcherHostTest, 
MediaStreamManagerTest,  VideoCaptureHostTest,
add the --use-fake-device-for-media-stream flag
to command line and depend on MSM reacting to it
via injecting a fake factory to VCM on creation.

Those tests also retrieve a weak reference to the
 Fake Factory to manipulate it.

FakeVideoCapture::SetFailNextCreate() and associated
member are removed, unused.


BUG=323913
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266380

[perkj_chrome, 2014-04-15 09:34:00]

initial comments.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:23: #include
"media/video/capture/fake_video_capture_device_factory.h"
You should not need these includes here.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:199:
DCHECK(IsOnDeviceThread());
This is not the IO thread right?

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:209: if
(!use_testing_device_source_) {
This is ugly. Always use a device factory but have different implementations.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:474: if
(!use_testing_device_source_)
dito

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:59: void
UseFakeDevice(scoped_ptr<media::VideoCaptureDeviceFactory> factory);
Why not just inject the factory in the ctor? Tests can inject fake version and
production the real one. You can have two ctors if you want the default ctor to
always create the default implementation of the factory.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:246: bool
use_testing_device_source_;
We want to get rid of this type of things in the production code. You should be
able to just have it in a test implemenetation of VideoCaptureDeviceFactory.

https://codereview.chromium.org/235353002/diff/40001/media/video/capture/vide...
File media/video/capture/video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.h:14: // owned by
VideoCaptureManager in IO Thread.
is That really true? I thought the VideoCaptureManager called these methods on a
special thread owned by MediaStreamManager since it may take a long time to
enumerate.

https://codereview.chromium.org/235353002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.h:22: virtual
VideoCaptureDevice* Create(
Add areal implementation of this interface as well. It can call the existing
static methods.

[mcasas, 2014-04-15 13:47:08]

perkj@ PTAL.

As discussed offline, going Factory all the way with a
factory-to-old-static-methods forwarding in video_capture_device_factory.cc.
Other platforms will follow in successive CLs.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:23: #include
"media/video/capture/fake_video_capture_device_factory.h"
On 2014/04/15 09:34:01, perkj wrote:
> You should not need these includes here.

Done.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:199:
DCHECK(IsOnDeviceThread());
On 2014/04/15 09:34:01, perkj wrote:
> This is not the IO thread right?

No, is a different one, Device thread, also known as
Audio Thread (where Audio Manager lives).

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:209: if
(!use_testing_device_source_) {
On 2014/04/15 09:34:01, perkj wrote:
> This is ugly. Always use a device factory but have different implementations.

Done.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:474: if
(!use_testing_device_source_)
On 2014/04/15 09:34:01, perkj wrote:
> dito

Done.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:59: void
UseFakeDevice(scoped_ptr<media::VideoCaptureDeviceFactory> factory);
On 2014/04/15 09:34:01, perkj wrote:
> Why not just inject the factory in the ctor? Tests can inject fake version and
> production the real one. You can have two ctors if you want the default ctor
to
> always create the default implementation of the factory.

Extended the ctor to accept a Factory. 
Some higher level UTs (MediaStreamDispatcherHostTest,
VideoCaptureHostTest, MediaStreamManagerTest), 
manipulate MediaStreamManager that in turn manages
VCM, they create the MSM which injects a "normal" factory
and later call UseFakeVideoDevice, which changes the
 factory to Fake/File. This is suboptimal.

https://codereview.chromium.org/235353002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.h:246: bool
use_testing_device_source_;
On 2014/04/15 09:34:01, perkj wrote:
> We want to get rid of this type of things in the production code. You should
be
> able to just have it in a test implemenetation of VideoCaptureDeviceFactory.

Done.

https://codereview.chromium.org/235353002/diff/40001/media/video/capture/vide...
File media/video/capture/video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.h:14: // owned by
VideoCaptureManager in IO Thread.
On 2014/04/15 09:34:01, perkj wrote:
> is That really true? I thought the VideoCaptureManager called these methods on
a
> special thread owned by MediaStreamManager since it may take a long time to
> enumerate.

MediaStreamManager lives between UI thread and IO thread. 
VideoCaptureManager lives between IO thread and Device thread.
The special thread for enumeration is the Device thread, for all
platforms except for Mac QTKit (AVFoundation is migrating).

https://codereview.chromium.org/235353002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.h:22: virtual
VideoCaptureDevice* Create(
On 2014/04/15 09:34:01, perkj wrote:
> Add areal implementation of this interface as well. It can call the existing
> static methods.

Done.

[mcasas, 2014-04-16 20:12:30]

Adding tommi@ to reviewers.

[tommi (sloooow) - chröme, 2014-04-17 07:37:58]

I have a few things to take care of before the weekend so I won't have time
to do a proper review just yet.  I think it's fine to have one reviewer for
now until the cl is no longer wip.


On Wed, Apr 16, 2014 at 8:12 PM, <mcasas@chromium.org> wrote:

> Adding tommi@ to reviewers.
>
> https://codereview.chromium.org/235353002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[perkj_chrome, 2014-04-17 08:39:09]

Nice cleanup. I think this will be great in the end.

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1443:
switches::kUseFileForFakeVideoCapture)) {
indentation

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1445:
scoped_ptr<media::VideoCaptureDeviceFactory>(
why not just take a pointer to the factory?

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1810: void
MediaStreamManager::UseFakeVideoDevice(
I think you should delete this method and in the unit test use
CommandLine::ForCurrentProcess()->AppendSwitch(
      switches::::kUseFakeDeviceForMediaStream);

See for ex MediaStreamAudioProcessorTest.

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1818:
audio_input_device_manager()->UseFakeDevice();
Actually - use the switch for this as well and remove the method. The question
is just if you should do it in this file on line 1435 or
theaudio_input_device_manager

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host_unittest.cc:289:
media_stream_manager_->UseFakeVideoDevice(
dito- use the switches.

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device.h (left):

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/fak...
media/video/capture/fake_video_capture_device.h:69: static bool
fail_next_create_;
Jipii!!! Thanks.

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.cc:11: return
VideoCaptureDevice::Create(device_name);
Please thread check here and below.

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:14: // owned by
VideoCaptureManager in IO Thread.
So this is obviously not true
VideoCaptureManager::GetAvailableDevicesInfoOnDeviceThread(. 
Please add thread checks in the default implementation and update this comment.

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:21: // TODO(mcasas): Return a
scoped_ptr to make ownership explicit.
This is a new class. Why leave this as a TODO?

[mcasas, 2014-04-23 06:20:27]

perkj@ PTAL.

The VCDF thread checker is hit in browser_tests, need to figure out why.

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1443:
switches::kUseFileForFakeVideoCapture)) {
On 2014/04/17 08:39:09, perkj wrote:
> indentation

Done.

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1445:
scoped_ptr<media::VideoCaptureDeviceFactory>(
On 2014/04/17 08:39:09, perkj wrote:
> why not just take a pointer to the factory?

The rest of MSM expects a scoped_ptr(), and is used for Register()/Unregister()
etc.

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1810: void
MediaStreamManager::UseFakeVideoDevice(
On 2014/04/17 08:39:09, perkj wrote:
> I think you should delete this method and in the unit test use
> CommandLine::ForCurrentProcess()->AppendSwitch(
>       switches::::kUseFakeDeviceForMediaStream);
> 
> See for ex MediaStreamAudioProcessorTest.

Ok, I was actually undecided between the switch option
and the method. Both are ugly 'cause the are means to
control the internal behaviour of VCM from outside, but
I guess the flag is marginally less ugly.

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1818:
audio_input_device_manager()->UseFakeDevice();
On 2014/04/17 08:39:09, perkj wrote:
> Actually - use the switch for this as well and remove the method. The question
> is just if you should do it in this file on line 1435 or
> theaudio_input_device_manager

Done in InitializeDeviceManagersOnIOThread().

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

https://codereview.chromium.org/235353002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host_unittest.cc:289:
media_stream_manager_->UseFakeVideoDevice(
On 2014/04/17 08:39:09, perkj wrote:
> dito- use the switches.

Done.

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device.h (left):

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/fak...
media/video/capture/fake_video_capture_device.h:69: static bool
fail_next_create_;
On 2014/04/17 08:39:09, perkj wrote:
> Jipii!!! Thanks.

Ack: the pleasure was all mine!

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.cc:11: return
VideoCaptureDevice::Create(device_name);
On 2014/04/17 08:39:09, perkj wrote:
> Please thread check here and below.

Check added to GetDeviceNames() and GetDeviceSupportedFormats().

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:14: // owned by
VideoCaptureManager in IO Thread.
On 2014/04/17 08:39:09, perkj wrote:
> So this is obviously not true
> VideoCaptureManager::GetAvailableDevicesInfoOnDeviceThread(. 
Correct, factory lives and operates in Device Thread, a.k.a. Audio Thread.

> Please add thread checks in the default implementation and update this
comment.

Done.

https://codereview.chromium.org/235353002/diff/100001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:21: // TODO(mcasas): Return a
scoped_ptr to make ownership explicit.
On 2014/04/17 08:39:09, perkj wrote:
> This is a new class. Why leave this as a TODO?

Changed here to scoped_ptr<> since it clarifies ownership of
the created VCD.

[tommi (sloooow) - chröme, 2014-04-23 14:33:58]

a big improvement!

https://codereview.chromium.org/235353002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/235353002/diff/140001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:84:
static_cast<media::FakeVideoCaptureDeviceFactory*>(
indent

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:12: const int32
kNumberOfFakeDevices = 2;
nit: I think we don't really need this constant

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:22: if
(device_name.id().compare(possible_id) == 0) {
nit: no {}

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:47: for (size_t i=0; i
< arraysize(supported_sizes); ++i) {
spaces around =

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.h:18: virtual
~FakeVideoCaptureDeviceFactory() {};
remove ;

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.h:30: int32
number_of_devices() {
nit: fits in one line?  ... actually, would it make sense to DCHECK correct
threadness here and in the setter?

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.h:35: int32
number_of_devices_;
why int32 and not int?

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fil...
File media/video/capture/file_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fil...
media/video/capture/file_video_capture_device_factory.cc:15: const char*
kFileVideoCaptureDeviceName =
const char kFileVideoCaptureDeviceName[]

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fil...
File media/video/capture/file_video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fil...
media/video/capture/file_video_capture_device_factory.h:15:
FileVideoCaptureDeviceFactory() {};
remove ; (and on the next line)

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.cc:24: };
no ; (and a few lines below)

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:20: virtual
~VideoCaptureDeviceFactory() {};
no ;

[mcasas, 2014-04-23 15:13:29]

tommi@, perkj@ PTAL.

A small area changed around FakeVCD::PopulateFormatsRoster(),
since FakeVCD does not see its own supported formats
anymore, I made the unittest plug all supported ones
(taken from the factory) explicitly.

https://codereview.chromium.org/235353002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/235353002/diff/140001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:84:
static_cast<media::FakeVideoCaptureDeviceFactory*>(
On 2014/04/23 14:33:59, tommi wrote:
> indent

Done.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:12: const int32
kNumberOfFakeDevices = 2;
On 2014/04/23 14:33:59, tommi wrote:
> nit: I think we don't really need this constant

The value itself is needed for some unittests, but does not belong here.

So locally is initialised to 1, and VCM unittest sets it to 2.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:22: if
(device_name.id().compare(possible_id) == 0) {
On 2014/04/23 14:33:59, tommi wrote:
> nit: no {}

Done.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:47: for (size_t i=0; i
< arraysize(supported_sizes); ++i) {
On 2014/04/23 14:33:59, tommi wrote:
> spaces around =

Done.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.h:18: virtual
~FakeVideoCaptureDeviceFactory() {};
On 2014/04/23 14:33:59, tommi wrote:
> remove ;

Done.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.h:30: int32
number_of_devices() {
On 2014/04/23 14:33:59, tommi wrote:
> nit: fits in one line?  ... actually, would it make sense to DCHECK correct
> threadness here and in the setter?

Added DCHECK(thread_checker_.CalledOnValidThread());
(checks for Device Thread).

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.h:35: int32
number_of_devices_;
On 2014/04/23 14:33:59, tommi wrote:
> why int32 and not int?

Copied from previous implementation.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fil...
File media/video/capture/file_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fil...
media/video/capture/file_video_capture_device_factory.cc:15: const char*
kFileVideoCaptureDeviceName =
On 2014/04/23 14:33:59, tommi wrote:
> const char kFileVideoCaptureDeviceName[]

Done.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fil...
File media/video/capture/file_video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/fil...
media/video/capture/file_video_capture_device_factory.h:15:
FileVideoCaptureDeviceFactory() {};
On 2014/04/23 14:33:59, tommi wrote:
> remove ; (and on the next line)

Done.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.cc:24: };
On 2014/04/23 14:33:59, tommi wrote:
> no ; (and a few lines below)

I must have been temporarily blind :)
Done.

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/140001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:20: virtual
~VideoCaptureDeviceFactory() {};
On 2014/04/23 14:33:59, tommi wrote:
> no ;

Done.

[perkj_chrome, 2014-04-24 14:43:32]

Very nice indeed.

Some more comments.

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:694:
video_capture_device_factory_->set_number_of_devices(number_of_fake_devices);
No need for this now right? Remove line 693 and 694.

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:855:
video_capture_device_factory_->set_number_of_devices(number_of_fake_devices);
dito, remove.

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1445: if
(CommandLine::ForCurrentProcess()->HasSwitch(
So to use kUseFileForFakeVideoCapture you also have to use
kUseFakeDeviceForMediaStream ? Is that intententional?

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1458:
scoped_ptr<media::VideoCaptureDeviceFactory>(
nit:indentation

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:225: // Device
creation factory injected from Media Stream Manager or from the test
 MediaStreamManager + please update the description. its always injected. I
think you should remove the comments here about the implementations since there
should be no assumptions of that in VideoCaptureManager now that the factory is
injected.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:16: for (int32 n = 0; n
< number_of_devices_; ++n) {
size_t instead of int32 or just int.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:27: for (int32 n = 0; n
< number_of_devices_; ++n) {
dito

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.h:13: 
Add a comment what this class does.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fil...
File media/video/capture/file_video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fil...
media/video/capture/file_video_capture_device_factory.h:11: 
Please add a comment what this does.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:16: // in Device Thread
(a.k.a. Audio Thread).
a.k.a. Audio Thread ?

[mcasas, 2014-04-24 20:20:25]

perkj@, tommi@ PTAL

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:694:
video_capture_device_factory_->set_number_of_devices(number_of_fake_devices);
On 2014/04/24 14:43:32, perkj wrote:
> No need for this now right? Remove line 693 and 694.

Done.

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:855:
video_capture_device_factory_->set_number_of_devices(number_of_fake_devices);
On 2014/04/24 14:43:32, perkj wrote:
> dito, remove.

Done.

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1445: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/04/24 14:43:32, perkj wrote:
> So to use kUseFileForFakeVideoCapture you also have to use
> kUseFakeDeviceForMediaStream ? Is that intententional?

Yes, it was indicated by fischman@.

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1458:
scoped_ptr<media::VideoCaptureDeviceFactory>(
On 2014/04/24 14:43:32, perkj wrote:
> nit:indentation

Done.

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/235353002/diff/180001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:225: // Device
creation factory injected from Media Stream Manager or from the test
On 2014/04/24 14:43:32, perkj wrote:
>  MediaStreamManager + please update the description. its always injected. I
> think you should remove the comments here about the implementations since
there
> should be no assumptions of that in VideoCaptureManager now that the factory
is
> injected.

Done.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:16: for (int32 n = 0; n
< number_of_devices_; ++n) {
On 2014/04/24 14:43:32, perkj wrote:
> size_t instead of int32 or just int.

int. 
size_t has troubles in Windows.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:27: for (int32 n = 0; n
< number_of_devices_; ++n) {
On 2014/04/24 14:43:32, perkj wrote:
> dito

Done.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.h:13: 
On 2014/04/24 14:43:32, perkj wrote:
> Add a comment what this class does.

Done.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fil...
File media/video/capture/file_video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fil...
media/video/capture/file_video_capture_device_factory.h:11: 
On 2014/04/24 14:43:32, perkj wrote:
> Please add a comment what this does.

Done.

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/vid...
File media/video/capture/video_capture_device_factory.h (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:16: // in Device Thread
(a.k.a. Audio Thread).
On 2014/04/24 14:43:32, perkj wrote:
> a.k.a. Audio Thread ? 

Yes, this is confusing but it seems that Audio thread was there earlier and was
reused as generic Device Thread [1], [2]. 
In Mac crashes, it identifies as Audio Thread.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...

[tommi (sloooow) - chröme, 2014-04-24 21:14:10]

lgtm

https://codereview.chromium.org/235353002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/235353002/diff/200001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1454:
video_capture_manager_ = new VideoCaptureManager(
too much indent

https://codereview.chromium.org/235353002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/235353002/diff/200001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:147: const
int32 kNumberOfFakeDevices = 2;
nit: static?

https://codereview.chromium.org/235353002/diff/200001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/200001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:13: :
number_of_devices_(1) {
4 spaces

[mcasas, 2014-04-25 08:33:10]

perkj@ PTAL
dalecurtis@: media/media.gyp  (/PTAL if you feel inclined to it).

https://codereview.chromium.org/235353002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/235353002/diff/200001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1454:
video_capture_manager_ = new VideoCaptureManager(
On 2014/04/24 21:14:11, tommi wrote:
> too much indent

Done.

https://codereview.chromium.org/235353002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/235353002/diff/200001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:147: const
int32 kNumberOfFakeDevices = 2;
On 2014/04/24 21:14:11, tommi wrote:
> nit: static?

Done. Needed anyway.

https://codereview.chromium.org/235353002/diff/200001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/200001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:13: :
number_of_devices_(1) {
On 2014/04/24 21:14:11, tommi wrote:
> 4 spaces

Done.

[perkj_chrome, 2014-04-25 08:43:30]

LGTM

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_factory.cc:16: for (int32 n = 0; n
< number_of_devices_; ++n) {
On 2014/04/24 20:20:26, mcasas wrote:
> On 2014/04/24 14:43:32, perkj wrote:
> > size_t instead of int32 or just int.
> 
> int. 
> size_t has troubles in Windows.
ok but you use size_t in GetDeviceSupportedFormats

https://codereview.chromium.org/235353002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/235353002/diff/220001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:147:
static const int32 kNumberOfFakeDevices = 2;
nit: This is only used once and can be declared just before use as const int32.

[mcasas, 2014-04-25 08:56:47]

On 2014/04/25 08:43:30, perkj wrote:
> LGTM
> 
>
https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
> File media/video/capture/fake_video_capture_device_factory.cc (right):
> 
>
https://codereview.chromium.org/235353002/diff/180001/media/video/capture/fak...
> media/video/capture/fake_video_capture_device_factory.cc:16: for (int32 n = 0;
n
> < number_of_devices_; ++n) {
> On 2014/04/24 20:20:26, mcasas wrote:
> > On 2014/04/24 14:43:32, perkj wrote:
> > > size_t instead of int32 or just int.
> > 
> > int. 
> > size_t has troubles in Windows.
> ok but you use size_t in GetDeviceSupportedFormats

Yes. the trouble comes from base::StringPrintf with a size_t in Win platforms.

> 
>
https://codereview.chromium.org/235353002/diff/220001/content/browser/rendere...
> File content/browser/renderer_host/media/video_capture_manager_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/235353002/diff/220001/content/browser/rendere...
> content/browser/renderer_host/media/video_capture_manager_unittest.cc:147:
> static const int32 kNumberOfFakeDevices = 2;
> nit: This is only used once and can be declared just before use as const
int32.

[DaleCurtis, 2014-04-25 19:56:33]

media.gyp lgtm

[mcasas, 2014-04-25 20:13:48]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:06:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/235353002/240001

[commit-bot: I haz the power, 2014-04-25 23:19:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:19:45]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[mcasas, 2014-04-26 06:24:40]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-04-26 06:25:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/235353002/240001

[commit-bot: I haz the power, 2014-04-26 22:15:55]

Change committed as 266380

[mcasas, 2014-04-28 15:01:19]

Publishing drafts. Please ignore.

https://codereview.chromium.org/235353002/diff/220001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/235353002/diff/220001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:147:
static const int32 kNumberOfFakeDevices = 2;
On 2014/04/25 08:43:31, perkj wrote:
> nit: This is only used once and can be declared just before use as const
int32.

Done.