VideoCaptureDeviceFactory: Change device enumeration to callback + QTKit enumerates in UI thread.

VideoCaptureDeviceFactory: change device enumeration to callback + QTKit enumerates in UI thread.

This CL changes the Enumeration to be based on 
a callback passed to the VideoCaptureDeviceFactory.
It is used for QTKit enumeration on UI thread:
this is speculatively better than trying to run
-performSelectorOnMainThread:withObject:waitUntilDone:

-- Description --

Currently VCM::EnumerateDevices() issues an internal 
PostTaskAndReplyWithResult where the first part
is GetAvailableDevicesInfoOnDeviceThread() on 
DeviceThread and the second is 
OnDevicesInfoEnumerated() in IO thread.

This CL changes this to a simple PostTask to the 
VCDFactory::EnumerateDeviceNames() with a callback
to VCM::ConsolidateDevicesInfoOnDeviceThread().
This method has a jump at the end to 
VCM::OnDevicesInfoEnumerated() on IO thread. So
what used to be a simple back-and-forth jump is
now a 3 hop process. 

This is used for QTKit to be able to override the
VCDF::EnumerateDeviceNames and run the device
enumeration in UI thread. 

Note 1: The UI thread is currently passed in Create()
method for all implementations. This is changed to
earlier: on VCDF::CreateFactory(); injected from 
MediaStreamManager; and only used in VCDFLinux 
(for ChromeOS) and in VCDFMac for QTKit enumeration.
This approach is cleaner since we don't send UI thread
references to derived classes that don't need it.

Note 2: VideoCaptureDeviceFactoryMac::Create()
used to have a search-and-find for |device_id| that
is removed; the search was a precaution for devices
that disappeared between first enumeration and use.
However, using a |device_name| that is gone would
just cause a fail on AllocateAndStart() anyhow, and
enumerating twice is very expensive.
 For compatibility reasons, this behaviour is kept
for AVFoundation devices and blacklisted QTKit
devices (that behave as a subset of AVF-type ones).



BUG=255552, 115327
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274518

[mcasas, 2014-05-22 12:19:08]

Ami: content/browser/renderer_host/media/* PTAL.

(and beyond if you want)

[mcasas, 2014-05-23 08:12:39]

tommi@ PTAL.

[mcasas, 2014-05-26 10:00:39]

perkj@ PTAL.

[perkj_chrome, 2014-05-26 11:49:42]

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:126: if
(stream_type == MEDIA_DESKTOP_VIDEO_CAPTURE) {
THis looks weird. Why return  devices_info_cache_ ? Isn-t that only for 
MEDIA_DEVICE_VIDEO_CAPTURE ? And is this ever called with
MEDIA_DESKTOP_VIDEO_CAPTURE ? If not - just dcheck the type.

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:133: //
"Media.VideoCaptureManager.GetAvailableDevicesInfoOnDeviceThreadTime");
this does not seem to have anything to do with what you are doing.

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:139:
base::Bind(&VideoCaptureManager::ConsolidateDevicesInfoOnDeviceThread,
To avoid the raw message loop pointer - how about a nested callback that you
bind to the current loop?

       media::BindToCurrentLoop( base::bind(
&VideoCaptureManager::OnDevicesInfoEnumerated, this, stream_type));

ConsolidateDevicesInfoOnDeviceThread then dont need to Post a task back again -
it just have to run the callback.

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:144: // OK to use
base::Unretained() since we own the VCDFactory.
What guarantee that the video capture manager is not destroyed? the bind to the
callback above?

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:510:
io_loop->PostTask(FROM_HERE,
Use a callback as input parameter instead and just run it here?

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

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:159:
base::MessageLoop* io_loop,
use MessageLoopProxy

https://codereview.chromium.org/294893006/diff/110001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/294893006/diff/110001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:29: // DCHECK that
ui_device_thread_ belongs to me.
remove this comment.

https://codereview.chromium.org/294893006/diff/110001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:127:
VideoCaptureDeviceFactory::EnumerateDeviceNames(callback);
This seems fragile. Remove GetDevicesNames and to the real implementation here
instead.

GetDeviceNames should now be made protected instead since its no longer intented
to be used from VideoCaptureManager right?

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

https://codereview.chromium.org/294893006/diff/110001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:46: virtual void
GetDeviceNames(VideoCaptureDevice::Names* device_names) = 0;
Should this really be pure virtual? I don-t think you should have an
implementation for this on Mac. Also make protected instead.
Please also write that this is used by the default implemenation of
EnumerateDeviceNames.

[mcasas, 2014-05-26 13:00:18]

perkj@ PTAL.

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:126: if
(stream_type == MEDIA_DESKTOP_VIDEO_CAPTURE) {
On 2014/05/26 11:49:42, perkj wrote:
> THis looks weird. Why return  devices_info_cache_ ? Isn-t that only for 
> MEDIA_DEVICE_VIDEO_CAPTURE ? And is this ever called with
> MEDIA_DESKTOP_VIDEO_CAPTURE ? If not - just dcheck the type.

This was a cleanup of previous code, where 
GetAvailableDevicesInfoOnDeviceThread() was called and
nothing was updated for this MEDIA_DESKTOP_VIDEO_CAPTURE...
(IOW I just made it obvious).

I'll leave only the l.130 DCHECK.

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:133: //
"Media.VideoCaptureManager.GetAvailableDevicesInfoOnDeviceThreadTime");
On 2014/05/26 11:49:42, perkj wrote:
> this does not seem to have anything to do with what you are doing.

Yes, this is an open question.

Before this CL, there was an UMA collection in 
GetAvailableDevicesInfoOnDeviceThread(), which was measuring
the time spent in synchronous GetDeviceNames().

If we think is interesting to collect that UMA still,
this TODO is the way to do it. Otherwise, I'll remove
the TODO, WDYT?

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:139:
base::Bind(&VideoCaptureManager::ConsolidateDevicesInfoOnDeviceThread,
On 2014/05/26 11:49:42, perkj wrote:
> To avoid the raw message loop pointer - how about a nested callback that you
> bind to the current loop?
> 
>        media::BindToCurrentLoop( base::bind(
> &VideoCaptureManager::OnDevicesInfoEnumerated, this, stream_type));
> 
> ConsolidateDevicesInfoOnDeviceThread then dont need to Post a task back again
-
> it just have to run the callback. 

Done.

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:144: // OK to use
base::Unretained() since we own the VCDFactory.
On 2014/05/26 11:49:42, perkj wrote:
> What guarantee that the video capture manager is not destroyed? the bind to
the
> callback above?

VCM outlives the VCDF, plus is reference counted via callback above.

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:510:
io_loop->PostTask(FROM_HERE,
On 2014/05/26 11:49:42, perkj wrote:
> Use a callback as input parameter instead and just run it here?

Done.

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

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:159:
base::MessageLoop* io_loop,
On 2014/05/26 11:49:42, perkj wrote:
> use MessageLoopProxy

Done.

https://codereview.chromium.org/294893006/diff/110001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/294893006/diff/110001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:29: // DCHECK that
ui_device_thread_ belongs to me.
On 2014/05/26 11:49:42, perkj wrote:
> remove this comment.

Done.

https://codereview.chromium.org/294893006/diff/110001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:127:
VideoCaptureDeviceFactory::EnumerateDeviceNames(callback);
On 2014/05/26 11:49:42, perkj wrote:
> This seems fragile. Remove GetDevicesNames and to the real implementation here
> instead.
> 
Done.

> GetDeviceNames should now be made protected instead since its no longer
intented
> to be used from VideoCaptureManager right?

Yes. Done.

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

https://codereview.chromium.org/294893006/diff/110001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.h:46: virtual void
GetDeviceNames(VideoCaptureDevice::Names* device_names) = 0;
On 2014/05/26 11:49:42, perkj wrote:
> Should this really be pure virtual? 

I think so: all derived classes are obliged to
provide an implementation.

> I don-t think you should have an implementation for this on Mac. 

But this is the point of this CL...
How would you do it instead?

> Also make protected instead.
Done.

> Please also write that this is used by the default implemenation of
> EnumerateDeviceNames.

Done.

[perkj_chrome, 2014-05-27 12:15:07]

I am happy with this if you address the below comments and I think its time for
a Mac reviewer...

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/110001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:133: //
"Media.VideoCaptureManager.GetAvailableDevicesInfoOnDeviceThreadTime");
On 2014/05/26 13:00:18, mcasas wrote:
> On 2014/05/26 11:49:42, perkj wrote:
> > this does not seem to have anything to do with what you are doing.
> 
> Yes, this is an open question.
> 
> Before this CL, there was an UMA collection in 
> GetAvailableDevicesInfoOnDeviceThread(), which was measuring
> the time spent in synchronous GetDeviceNames().
> 
> If we think is interesting to collect that UMA still,
> this TODO is the way to do it. Otherwise, I'll remove
> the TODO, WDYT?

Was that something you added because you wanted it or was it a request from
someone else? If someone else rely on it you must fix it in this cl. Your call.

https://codereview.chromium.org/294893006/diff/130001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/130001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:134:
base::Callback<void(media::VideoCaptureDevice::Names&)>
This magic would benefit from a comment on what we are doing.

https://codereview.chromium.org/294893006/diff/130001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:144: // OK to use
base::Unretained() since we own the VCDFactory.
Maybe add "and |this| is bound in devices_enumerated_callback."

[mcasas, 2014-05-27 15:23:40]

tommi@ PTAL.

dalecurtis@ what do you think about this approach?

https://codereview.chromium.org/294893006/diff/130001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/130001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:134:
base::Callback<void(media::VideoCaptureDevice::Names&)>
On 2014/05/27 12:15:08, perkj wrote:
> This magic would benefit from a comment on what we are doing.

Done.

https://codereview.chromium.org/294893006/diff/130001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:144: // OK to use
base::Unretained() since we own the VCDFactory.
On 2014/05/27 12:15:08, perkj wrote:
> Maybe add "and |this| is bound in devices_enumerated_callback."

Done.

[DaleCurtis, 2014-05-27 17:51:07]

I'm far enough removed from this code to not really have a useful opinion :)  I
like that you're allowing injection of a loop to the manager though.  My
questions from the description would be: What does speculatively better mean? 
What improvement are you gaining by switching to a 3-hop process?

[mcasas, 2014-05-28 08:04:49]

On 2014/05/27 17:51:07, DaleCurtis wrote:
> I'm far enough removed from this code to not really have a useful opinion :) 
I
> like that you're allowing injection of a loop to the manager though.  My
> questions from the description would be: What does speculatively better mean? 

Speculatively better refers to crash-count. We've noticed that the call
to -[QTCaptureDevice EnumerateDevicesWithType:] tends to work 
fine when run from the DeviceMonitorMac (in UI thread) while it
crashes every now and then when run from VCDQTKit using 
-performSelectorOnMainThread:... . Since this crash has been around
for quite some time, I think it's worth trying to run the same 
enumeration directly on UI thread.

> What improvement are you gaining by switching to a 3-hop process?
The 3 step process is in this case a by-product of shifting the
device enumeration to happen via callback. I personally
find it positive that it also pushes the synchronous  enumeration 
part inside the VCDFactory where it belongs. 

A side note: this CL also does a bit of cleanup in proximal areas.

[mcasas, 2014-05-29 13:47:14]

perkj@, does "happy" mean LGTY?

[tommi (sloooow) - chröme, 2014-05-30 12:37:51]

https://codereview.chromium.org/294893006/diff/150001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/150001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:132:
base::Callback<void(media::VideoCaptureDevice::Names&)>
either const& or *

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

https://codereview.chromium.org/294893006/diff/150001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:164:
media::VideoCaptureDevice::Names& names_snapshot);
const
(or if it is meant to be writable, pass by pointer)

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:58: public
base::RefCounted<DeviceEnumerationListener>{
space before {

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:63: void
EnumeratedDevicesCallback(media::VideoCaptureDevice::Names& names) {
const

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:63: void
EnumeratedDevicesCallback(media::VideoCaptureDevice::Names& names) {
why is this method needed? (add a comment?)
should it be virtual? (override?)

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:68:
~DeviceEnumerationListener() {}
virtual?

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:99:
.WillOnce(SaveArg<0>(device_names));
indent

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:28: static
VideoCaptureDevice::Names EnumerateDevicesUsingQTKit(){
space before {

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:133:
base::Bind(&EnumerateDevicesUsingQTKit),
indent

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/vid...
File media/video/capture/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/vid...
media/video/capture/video_capture_device_unittest.cc:148:
.WillOnce(SaveArg<0>(device_names));
indent

[mcasas, 2014-05-30 14:08:49]

tommi@ PTAL.
perkj@ ping.

https://codereview.chromium.org/294893006/diff/150001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/150001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:132:
base::Callback<void(media::VideoCaptureDevice::Names&)>
On 2014/05/30 12:37:51, tommi wrote:
> either const& or *

const&, done.

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

https://codereview.chromium.org/294893006/diff/150001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:164:
media::VideoCaptureDevice::Names& names_snapshot);
On 2014/05/30 12:37:51, tommi wrote:
> const
> (or if it is meant to be writable, pass by pointer)

Made it const& and used inside a local copy for
modifications. I think it's clearer that way, 
otherwise it permeates to the the caller
that ConsolidateDevicesInfoOnDeviceThread() is 
going to manipulate/modify/destroy the Names.

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:58: public
base::RefCounted<DeviceEnumerationListener>{
On 2014/05/30 12:37:51, tommi wrote:
> space before {

Done.

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:63: void
EnumeratedDevicesCallback(media::VideoCaptureDevice::Names& names) {
On 2014/05/30 12:37:51, tommi wrote:
> why is this method needed? (add a comment?)
> should it be virtual? (override?)

It isn't, so removed.

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:63: void
EnumeratedDevicesCallback(media::VideoCaptureDevice::Names& names) {
On 2014/05/30 12:37:51, tommi wrote:
> const

const& in the previous MOCK_METHOD.

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:68:
~DeviceEnumerationListener() {}
On 2014/05/30 12:37:51, tommi wrote:
> virtual?

Done.

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/fak...
media/video/capture/fake_video_capture_device_unittest.cc:99:
.WillOnce(SaveArg<0>(device_names));
On 2014/05/30 12:37:51, tommi wrote:
> indent

Done.

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:28: static
VideoCaptureDevice::Names EnumerateDevicesUsingQTKit(){
On 2014/05/30 12:37:51, tommi wrote:
> space before {

Done.

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:133:
base::Bind(&EnumerateDevicesUsingQTKit),
On 2014/05/30 12:37:51, tommi wrote:
> indent

Done.

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/vid...
File media/video/capture/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/294893006/diff/150001/media/video/capture/vid...
media/video/capture/video_capture_device_unittest.cc:148:
.WillOnce(SaveArg<0>(device_names));
On 2014/05/30 12:37:51, tommi wrote:
> indent

Done.

[tommi (sloooow) - chröme, 2014-05-30 14:16:38]

https://codereview.chromium.org/294893006/diff/170001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/170001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:484:
media::VideoCaptureDevice::Names current_devices(names_snapshot);
ah, so if the goal is actually to modify the array, why don't we create a copy
of the snapshot on the IO thread where we post a task to execute this one?
(pass via scoped_ptr<>.Pass())

As is, this will be the third copy of the snapshot and it's actually a bit
confusing to be sending a reference to a member variable that is only allowed to
be touched on the IO thread, to this thread.  It works, but only because the
callback actually creates a new copy behind the scenes.

[mcasas, 2014-05-30 19:42:47]

PTAL.

https://codereview.chromium.org/294893006/diff/170001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/294893006/diff/170001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:484:
media::VideoCaptureDevice::Names current_devices(names_snapshot);
On 2014/05/30 14:16:38, tommi wrote:
> ah, so if the goal is actually to modify the array, why don't we create a copy
> of the snapshot on the IO thread where we post a task to execute this one?
> (pass via scoped_ptr<>.Pass())
> 
> As is, this will be the third copy of the snapshot and it's actually a bit
> confusing to be sending a reference to a member variable that is only allowed
to
> be touched on the IO thread, to this thread.  It works, but only because the
> callback actually creates a new copy behind the scenes.

I changed |names_snapshot| to scoped_ptr<>, so 
VideoCaptureDeviceFactory actually passes the
enumerated devices list to the VideoCaptureManager;
like this there is no extra copy and ownership is
clear.

[tommi (sloooow) - chröme, 2014-05-30 21:23:34]

thanks! lgtm

[mcasas, 2014-05-31 11:48:11]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-31 11:48:57]

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

[commit-bot: I haz the power, 2014-05-31 13:48:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/11376)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-31 13:48:48]

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

[commit-bot: I haz the power, 2014-05-31 13:48:49]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[mcasas, 2014-05-31 14:36:02]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-31 14:36:34]

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

[commit-bot: I haz the power, 2014-05-31 16:35:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-31 16:36:37]

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

[commit-bot: I haz the power, 2014-05-31 16:36:38]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[mcasas, 2014-05-31 19:31:39]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-31 19:32:18]

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

[commit-bot: I haz the power, 2014-05-31 22:13:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-01 00:01:15]

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

[commit-bot: I haz the power, 2014-06-01 00:01:15]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[mcasas, 2014-06-01 10:13:02]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-06-01 10:13:11]

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

[tommi (sloooow) - chröme, 2014-06-01 11:25:21]

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:76:
GetDeviceNames(device_names.get());
Doesn't this also enumerate QTKit devices? (Blackmagic)

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:84: return
scoped_ptr<VideoCaptureDevice>();
If this check indeed only looks at devices from avfoundation, then won't this
cause the bm devices to be blocked?

[mcasas, 2014-06-01 11:44:45]

The CQ bit was unchecked by mcasas@chromium.org

[mcasas, 2014-06-02 13:19:03]

tommi@ PTAL.

A bit of an extensive answer, hope I addressed your comments.

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:76:
GetDeviceNames(device_names.get());
On 2014/06/01 11:25:22, tommi wrote:
> Doesn't this also enumerate QTKit devices? (Blackmagic) 

Yes, if present in the system.

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:84: return
scoped_ptr<VideoCaptureDevice>();
On 2014/06/01 11:25:22, tommi wrote:
> If this check indeed only looks at devices from avfoundation, then won't this
> cause the bm devices to be blocked? 

Just to be clear of the process, there are currently 2 enumerations
from the VCD family side: One is the common EnumerateDeviceNames,
that's cached in VCManager, and this secondary one on device Create().
This secondary check is in place to avoid trying to Create() a device
that has been disconnected from the system after enumeration;
this is no longer a possibility since the DeviceMonitorMac is listening
to connect/disconnect (triggering an enumeration), hence the time
span between enumeration and Create() is quite reduced; even if a
non-connected device is Create()d, the subsequent call to 
AllocateAndStart() would fail (they are done almost 
back-to-back [1] and [2]).

Since QTKit is very sensitive to enumerations, the fewer the better,
so this CL removes this second one.  AVFoundation double checking
is, however, left in place so that the functionality (and the 
unittest) stay the same, but I think we could remove this secondary
enumeration, which indeed might even trigger QTKit enumeration
of blacklisted devices.

Blacklisted devices have their own path inside AVFoundation 
GetDeviceNames, i.e. they don't use the normal QTKit path, so
they should still work fine.

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

[tommi (sloooow) - chröme, 2014-06-03 08:52:10]

lgtm.  Please check that my understanding below is correct.

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:84: return
scoped_ptr<VideoCaptureDevice>();
On 2014/06/02 13:19:04, mcasas wrote:
> On 2014/06/01 11:25:22, tommi wrote:
> > If this check indeed only looks at devices from avfoundation, then won't
this
> > cause the bm devices to be blocked? 
> 
> Just to be clear of the process, there are currently 2 enumerations
> from the VCD family side: One is the common EnumerateDeviceNames,
> that's cached in VCManager, and this secondary one on device Create().
> This secondary check is in place to avoid trying to Create() a device
> that has been disconnected from the system after enumeration;
> this is no longer a possibility since the DeviceMonitorMac is listening
> to connect/disconnect (triggering an enumeration), hence the time
> span between enumeration and Create() is quite reduced; even if a
> non-connected device is Create()d, the subsequent call to 
> AllocateAndStart() would fail (they are done almost 
> back-to-back [1] and [2]).
> 
> Since QTKit is very sensitive to enumerations, the fewer the better,
> so this CL removes this second one.  AVFoundation double checking
> is, however, left in place so that the functionality (and the 
> unittest) stay the same, but I think we could remove this secondary
> enumeration, which indeed might even trigger QTKit enumeration
> of blacklisted devices.
> 
> Blacklisted devices have their own path inside AVFoundation 
> GetDeviceNames, i.e. they don't use the normal QTKit path, so
> they should still work fine.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

OK, thanks.

So just to recap and verify that my understanding is correct:

1.  In this scope we enumerate both AVFoundation devices *and* Blackmagic
devices (via qtkit).

2. This check (|if (it == device_names->end())|) will never be true for
Blackmagic devices.

[mcasas, 2014-06-03 13:56:45]

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_factory_mac.mm (right):

https://codereview.chromium.org/294893006/diff/280001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:84: return
scoped_ptr<VideoCaptureDevice>();
On 2014/06/03 08:52:11, tommi wrote:
> On 2014/06/02 13:19:04, mcasas wrote:
> > On 2014/06/01 11:25:22, tommi wrote:
> > > If this check indeed only looks at devices from avfoundation, then won't
> this
> > > cause the bm devices to be blocked? 
> > 
> > Just to be clear of the process, there are currently 2 enumerations
> > from the VCD family side: One is the common EnumerateDeviceNames,
> > that's cached in VCManager, and this secondary one on device Create().
> > This secondary check is in place to avoid trying to Create() a device
> > that has been disconnected from the system after enumeration;
> > this is no longer a possibility since the DeviceMonitorMac is listening
> > to connect/disconnect (triggering an enumeration), hence the time
> > span between enumeration and Create() is quite reduced; even if a
> > non-connected device is Create()d, the subsequent call to 
> > AllocateAndStart() would fail (they are done almost 
> > back-to-back [1] and [2]).
> > 
> > Since QTKit is very sensitive to enumerations, the fewer the better,
> > so this CL removes this second one.  AVFoundation double checking
> > is, however, left in place so that the functionality (and the 
> > unittest) stay the same, but I think we could remove this secondary
> > enumeration, which indeed might even trigger QTKit enumeration
> > of blacklisted devices.
> > 
> > Blacklisted devices have their own path inside AVFoundation 
> > GetDeviceNames, i.e. they don't use the normal QTKit path, so
> > they should still work fine.
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> > [2]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> OK, thanks.
> 
> So just to recap and verify that my understanding is correct:
> 
> 1.  In this scope we enumerate both AVFoundation devices *and* Blackmagic
> devices (via qtkit).

That's correct.

> 
> 2. This check (|if (it == device_names->end())|) will never be true for
> Blackmagic devices.

Correct, it should not verify as long as the Blackmagic device is still
connected in the system (from the moment it was first enumerated).

[mcasas, 2014-06-03 13:56:53]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-06-03 13:57:51]

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

[commit-bot: I haz the power, 2014-06-03 13:59:19]

Change committed as 274518