Mac Video Capture Device: split VCD into VCD and Factory.

Mac Video Capture Device: split VCD into VCD and Factory.

VideoCaptureDeviceMac includes factory and non-factory parts.
This CL splits them into VideoCaptureDeviceMac and 
VideoCaptureDeviceFactoryMac. The latter inherits the previous
class' static methods: Create(), GetDeviceNames() and 
GetDeviceSupportedFormats().

All video factory code previously in MediaStreamManager is
moved into VideoCaptureFactory. This includes the use
of the flag |kUseFakeDeviceForMediaStream|.
This flag is moved correspondingly into media_switches.cc --
(but note that this flag is still used in MediaStreamManager for 
the Fake Audio parts). File media_switches.cc is
included in several test files where the flag is used.


VideoCaptureDeviceTest is splitted as well into:
a) Tests that were exercising only the FakeVCD 
(FakeVideoCaptureDeviceTest).
b) All other tests, that use the underlying OS webcam.
This VideoCaptureDeviceTest gets a Factory and uses it
instead of static methods.

A unit test is added to VCDFMac, doing little for the moment
but I'm planning to add support for testing at least the
blacklisting -- req from rsesek@ in another CL.

BUG=288562, 323913, 255552
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269271

[mcasas, 2014-05-05 16:45:58]

perkj@ could you plz have a first look and tell if I'm going in the right
direction? 

I'm particularly interested in your opinion on changes in
media_stream_manager.cc and video_capture_device_factory.cc.

[perkj_chrome, 2014-05-06 06:44:46]

Yes I think this is the correct path.

https://codereview.chromium.org/265263004/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/265263004/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1444: if
(CommandLine::ForCurrentProcess()->HasSwitch(
You can move this too into your Create method.- ie, Create decide what factory
to use, including the fakes.

https://codereview.chromium.org/265263004/diff/80001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/265263004/diff/80001/media/media.gyp#newcode1192
media/media.gyp:1192: ['OS=="mac"', {
Why only this unittest and not the other platform specifics? Check how other
platform dependent unittests are added. This might be correct...

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/mac/...
File media/video/capture/mac/video_capture_device_factory_mac.h (right):

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.h:26:
VideoCaptureFormats* supported_formats) OVERRIDE;
DISALLOW_COPY_AND_ASSIGN

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

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:62: if
(AVFoundationGlue::IsAVFoundationSupported()) {
How about we do this check already in VideoCaptureDeviceFactory::Create and
create separate factories depending on AVFoundation or QTKit?

Then you don't need to check in each method.

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
File media/video/capture/video_capture_device_factory.cc (right):

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:6: 
#if defined(OS_MACOSX)
#include ....

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:18: #endif
Why not create the fake ones here too?

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:32: // Remove as well the
call to the VideoCaptureDevice static method.
no need for this since you override VideoCaptureDeviceFactory::Create in teh mac
version.

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:45: #if !defined(OS_MACOSX)
dito

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:55: #if !defined(OS_MACOSX)
dito

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

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.h:19: static
scoped_ptr<VideoCaptureDeviceFactory> Create();
CreateFactory to not mix with the Create method used for creating devices?

[mcasas, 2014-05-06 07:26:29]

perkj@ PTAL.

Please note the otherwise added comment:
...  "the use of the flag |kUseFakeDeviceForMediaStream|. 
This flag is moved correspondingly into media_switches.cc -- 
(but note that this flag is still used in MediaStreamManager for 
the Fake Audio parts). Moving this flag into media_switches.cc 
forces modification of a bunch of unittests, to include it. "

https://codereview.chromium.org/265263004/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/265263004/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1444: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/05/06 06:44:46, perkj wrote:
> You can move this too into your Create method.- ie, Create decide what factory
> to use, including the fakes.

Great idea. Done so, and with it I moved the flag
|kUseFakeDeviceForMediaStream| into media_switches --
note that this flag is still used in MediaStreamManager for 
the Fake Audio parts.

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/mac/...
File media/video/capture/mac/video_capture_device_factory_mac.h (right):

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.h:26:
VideoCaptureFormats* supported_formats) OVERRIDE;
On 2014/05/06 06:44:46, perkj wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

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

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_factory_mac.mm:62: if
(AVFoundationGlue::IsAVFoundationSupported()) {
On 2014/05/06 06:44:46, perkj wrote:
> How about we do this check already in VideoCaptureDeviceFactory::Create and
> create separate factories depending on AVFoundation or QTKit?
> 
> Then you don't need to check in each method.

I'd rather do that in a later CL, so this one is concerned with
moving this around without modifying the functionality.

Performance-wise the IsAVFoundationSupported() returns a
static bool, so shouldn't be too bad.

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
File media/video/capture/video_capture_device_factory.cc (right):

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:6: 
On 2014/05/06 06:44:46, perkj wrote:
> #if defined(OS_MACOSX)
> #include ....

Done.

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:18: #endif
On 2014/05/06 06:44:46, perkj wrote:
> Why not create the fake ones here too?

Done.

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:32: // Remove as well the
call to the VideoCaptureDevice static method.
On 2014/05/06 06:44:46, perkj wrote:
> no need for this since you override VideoCaptureDeviceFactory::Create in teh
mac
> version.

I completely agree but the linker gives a missing symbol complaint
if I don't define-out the usage of VideoCaptureDevice::Create().

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:45: #if !defined(OS_MACOSX)
On 2014/05/06 06:44:46, perkj wrote:
> dito

See before.

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.cc:55: #if !defined(OS_MACOSX)
On 2014/05/06 06:44:46, perkj wrote:
> dito

See before.

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

https://codereview.chromium.org/265263004/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_device_factory.h:19: static
scoped_ptr<VideoCaptureDeviceFactory> Create();
On 2014/05/06 06:44:46, perkj wrote:
> CreateFactory to not mix with the Create method used for creating devices?

Done.

[perkj_chrome, 2014-05-06 11:32:42]

Can you upload a new cl with git cl upload --similarity=20 to make it easier to
see that you have not changed the mac enumeration code? 

I think this looks good but I have not reviewed the enumeration code since you
said its unchanged.

Would it make sence to include media_swithes.h in content_switches.h?

https://codereview.chromium.org/265263004/diff/120001/media/base/media_switch...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/265263004/diff/120001/media/base/media_switch...
media/base/media_switches.cc:90: // video part is actually used in
VideoCaptureDeviceFactory.
nit: remove the sentence where its used.

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

https://codereview.chromium.org/265263004/diff/120001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.cc:52: // TODO(mcasas): Remove
the #if parts when all platforms have splitted the
You don't need all these Ifdefs now right?  All methods are virtual.

[mcasas, 2014-05-06 12:47:20]

perkj@ PTAL.

Taken in your suggestion of including media_switches.h 
into content_switches.h.

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

https://codereview.chromium.org/265263004/diff/100001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.cc:23: // line flags are
present, otherwise use a normal device factory.
Rewrite part on "Fake Audio Device".

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

https://codereview.chromium.org/265263004/diff/100001/media/video/capture/vid...
media/video/capture/video_capture_device_unittest.cc:99:
video_capture_device_factory_(new VideoCaptureDeviceFactory()) {}
Should use VideoCaptureDeviceFactory::CreateFactory()

https://codereview.chromium.org/265263004/diff/120001/media/base/media_switch...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/265263004/diff/120001/media/base/media_switch...
media/base/media_switches.cc:90: // video part is actually used in
VideoCaptureDeviceFactory.
On 2014/05/06 11:32:43, perkj wrote:
> nit: remove the sentence where its used.

Done.

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

https://codereview.chromium.org/265263004/diff/120001/media/video/capture/vid...
media/video/capture/video_capture_device_factory.cc:52: // TODO(mcasas): Remove
the #if parts when all platforms have splitted the
On 2014/05/06 11:32:43, perkj wrote:
> You don't need all these Ifdefs now right?  All methods are virtual.

Done.

[perkj_chrome, 2014-05-06 18:25:59]

Looks good with the below fixed. Just wondering if its ok to inlcude
media_switches in content_switches.

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

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:38:
VideoCaptureDevice::Names device_names;
yes this check seems unneccessary. That must be handled by init and/or new
VideoCaptureDeviceMac anway right? So why not just remove the todo and the code.

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:46: return
scoped_ptr<VideoCaptureDevice>();
This will leak the memory used by  capture_device unless you make it a
scoped_ptr

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:54: return
scoped_ptr<VideoCaptureDevice>(capture_device);
captur_device.pass

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_mac.mm:44: // TODO(mcasas): Remove
the static methods implementations all platforms have
nit: fix readable sentence.

[mcasas, 2014-05-07 06:38:36]

jochen@: content/public/common/DEPS

context: moved a flag from content_switches.h into media_switches.h, and to
avoid having to include the last one in many test files, included the latter in
the former, and needed to add this DEPS rule, is it conforming to Chomium-isms?
Thanks!

[jochen (gone - plz use gerrit), 2014-05-07 06:58:02]

On 2014/05/07 06:38:36, mcasas wrote:
> jochen@: content/public/common/DEPS
> 
> context: moved a flag from content_switches.h into media_switches.h, and to
> avoid having to include the last one in many test files, included the latter
in
> the former, and needed to add this DEPS rule, is it conforming to
Chomium-isms?
> Thanks!

i think it's preferable to include the media switches header where it's used

[mcasas, 2014-05-07 07:49:11]

perkj@ LGTY?

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

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:38:
VideoCaptureDevice::Names device_names;
On 2014/05/06 18:26:00, perkj wrote:
> yes this check seems unneccessary. That must be handled by init and/or new
> VideoCaptureDeviceMac anway right? So why not just remove the todo and the
code.

Done.

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:46: return
scoped_ptr<VideoCaptureDevice>();
On 2014/05/06 18:26:00, perkj wrote:
> This will leak the memory used by  capture_device unless you make it a
> scoped_ptr

Done.

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_factory_mac.mm:54: return
scoped_ptr<VideoCaptureDevice>(capture_device);
On 2014/05/06 18:26:00, perkj wrote:
> captur_device.pass

Done.

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/265263004/diff/140001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_mac.mm:44: // TODO(mcasas): Remove
the static methods implementations all platforms have
On 2014/05/06 18:26:00, perkj wrote:
> nit: fix readable sentence.

Done.

[perkj_chrome, 2014-05-07 10:46:48]

LGTM with a nit

https://codereview.chromium.org/265263004/diff/160001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/265263004/diff/160001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_mac.mm:44: // TODO(mcasas): Remove
the following static methods when they are no longer
nit: move to after GetBestMatchSupportedResolution

[perkj_chrome, 2014-05-07 10:47:32]

https://codereview.chromium.org/265263004/diff/160001/content/public/common/DEPS
File content/public/common/DEPS (right):

https://codereview.chromium.org/265263004/diff/160001/content/public/common/D...
content/public/common/DEPS:4: ],
revert this change

[mcasas, 2014-05-07 12:18:23]

jochen@: 6 files, namely:
chrome/browser/{apps,media}/*test.cc
content/public/common/content_switches.{h,cc} 

dalecurtis@: media/base/media_switches.{h,cc} and media/media.gyp


Context for both of you guys: I moved a flag from content_switches.h into
media_switches.h. A number of tests got the former #include'd due to this (just
a +1 diff).

https://codereview.chromium.org/265263004/diff/160001/content/public/common/DEPS
File content/public/common/DEPS (right):

https://codereview.chromium.org/265263004/diff/160001/content/public/common/D...
content/public/common/DEPS:4: ],
On 2014/05/07 10:47:32, perkj wrote:
> revert this change

Done.

https://codereview.chromium.org/265263004/diff/160001/media/video/capture/mac...
File media/video/capture/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/265263004/diff/160001/media/video/capture/mac...
media/video/capture/mac/video_capture_device_mac.mm:44: // TODO(mcasas): Remove
the following static methods when they are no longer
On 2014/05/07 10:46:49, perkj wrote:
> nit: move to after GetBestMatchSupportedResolution

Done.

[jochen (gone - plz use gerrit), 2014-05-07 12:45:27]

lgtm

[DaleCurtis, 2014-05-07 17:10:29]

{media.gyp,media_switches} lgtm

[mcasas, 2014-05-07 17:36:02]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-07 17:39:53]

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

[commit-bot: I haz the power, 2014-05-07 23:41:47]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 04:21:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 05:02:00]

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

[commit-bot: I haz the power, 2014-05-08 05:02:01]

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

[mcasas, 2014-05-08 06:38:18]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-08 06:40:06]

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

[mcasas, 2014-05-08 09:34:19]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-08 09:38:08]

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

[commit-bot: I haz the power, 2014-05-08 22:37:29]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 13:53:37]

Change committed as 269271

[mcasas, 2014-05-09 14:28:14]

A revert of this CL has been created in
https://codereview.chromium.org/273063002/ by mcasas@chromium.org.

The reason for reverting is: Some backoffice bots broke due to
|kUseFakeDeviceForMediaStream| not being correctly defined - reverting until M36
storm passes and then I'll figure those out..