VideoCaptureDeviceWin: Extract class-static method into a Factory (both MF/DS)

VideoCaptureDeviceWin: Extract class-static method into a Factory (both MF/DS)

This CL extends the separation Factory/device
initiated in the other CLs part of the bug. Aside
from code cleanup, it allows for better testing
(via dependency injection) of the factory parts
(this is specially important for blacklisted devices,
of which there are none in Win ATM). It can also
be used to modify the GetDeviceNames() method to
an asynchronous API.

Both VideoCaptureDeviceWin and VideoCaptureDeviceMFwin 
(using DirectShow and MediaFoundation APIs resp.),
have a number of static methods Create(),
GetDeviceNames() and GetDeviceSupportedFormats().
Plus, there are overarching versions of those
at the VideoCaptureDevice level, that do library
initialisation and appropriate forwarding to one
or the other implementation. This CL extracts all
of those class-static methods and moves them to
a single VideoCaptureDeviceFactoryWin.

ScopedMediaType, previously file-level data type
moves to the public def of VideoCaptureDeviceWin
since is used in the Factory.

Every file-static function and variable that could,
has been moved to the Factory file. Previous 
file-level static functions that are used in the
Factory and the VCD have been changed to VCD
static methods and used in both ends.

TBR= dalecurtis@chromium.org (media/media.gyp)

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

[mcasas, 2014-05-15 12:03:55]

tommi@ PTAL.

Although methods in VCDFWin come from either VCDWin or
VCDMFWin, they only show compared to VCDWin, sorry about
that.

[tommi (sloooow) - chröme, 2014-05-15 13:56:47]

On 2014/05/15 12:03:55, mcasas wrote:
> tommi@ PTAL.
> 
> Although methods in VCDFWin come from either VCDWin or
> VCDMFWin, they only show compared to VCDWin, sorry about
> that.

I'm reviewing now, but it would be good if you can update the description to
include a bit more information that clarifies the benefits of this change.  It
seems rather big, and I'm not seeing much value in it right now.

[tommi (sloooow) - chröme, 2014-05-15 15:18:33]

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:45: // initializing
MF.
See the comments where LoadMediaFoundationDlls is called which explains why this
list is here.  It is not here to initialize MF actually, so you can't remove it
(unless you've otherwise addressed the problems described below first).

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.cc:10: #include
"base/lazy_instance.h" // remove me !!!
ping :)

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_mf_win.h (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.h:25: using
base::win::ScopedComPtr;
nit: I *think* we try to avoid using "using" in header files.

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.h:41: bool
Init(ScopedComPtr<IMFMediaSource> source);
const &

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:198: NOTIMPLEMENTED();
not implemented?  am I perhaps looking at a strange diff?

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:204: NOTIMPLEMENTED();
same here... I'm a bit confused (same for Create below)

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_win.h (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.h:28: using
base::win::ScopedComPtr;
if you remove the other one, then please this one as well.

[mcasas, 2014-05-15 16:03:23]

tommi@ PTAL.

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:45: // initializing
MF.
On 2014/05/15 15:18:33, tommi wrote:
> See the comments where LoadMediaFoundationDlls is called which explains why
this
> list is here.  It is not here to initialize MF actually, so you can't remove
it
> (unless you've otherwise addressed the problems described below first).

Works for me!

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_mf_win.cc (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.cc:10: #include
"base/lazy_instance.h" // remove me !!!
On 2014/05/15 15:18:33, tommi wrote:
> ping :)

Done.

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_mf_win.h (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.h:25: using
base::win::ScopedComPtr;
On 2014/05/15 15:18:33, tommi wrote:
> nit: I *think* we try to avoid using "using" in header files.

I wasn't sure of this but now I am. Removing.

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.h:41: bool
Init(ScopedComPtr<IMFMediaSource> source);
On 2014/05/15 15:18:33, tommi wrote:
> const &

Done.

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:198: NOTIMPLEMENTED();
On 2014/05/15 15:18:33, tommi wrote:
> not implemented?  am I perhaps looking at a strange diff?

It's missing a comment (like in Mac/Linux/Win). 
VideoCaptureDeviceFactory::CreateFactory() should create
the appropriate platform factory, but until all platforms
are landed, the VCDF::Create(), GetDeviceNames()
and GetDeviceSupportedFormat() are still referenced.

I'll clean these in the next CL, FWIW.

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:204: NOTIMPLEMENTED();
On 2014/05/15 15:18:33, tommi wrote:
> same here... I'm a bit confused (same for Create below)

Done.

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_win.h (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.h:28: using
base::win::ScopedComPtr;
On 2014/05/15 15:18:33, tommi wrote:
> if you remove the other one, then please this one as well.

Done.

[tommi (sloooow) - chröme, 2014-05-19 10:51:36]

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:198: NOTIMPLEMENTED();
On 2014/05/15 16:03:23, mcasas wrote:
> On 2014/05/15 15:18:33, tommi wrote:
> > not implemented?  am I perhaps looking at a strange diff?
> 
> It's missing a comment (like in Mac/Linux/Win). 
> VideoCaptureDeviceFactory::CreateFactory() should create
> the appropriate platform factory, but until all platforms
> are landed, the VCDF::Create(), GetDeviceNames()
> and GetDeviceSupportedFormat() are still referenced.
> 
> I'll clean these in the next CL, FWIW.

Can we #ifdef this so that it's clear on what platforms these are needed?

https://codereview.chromium.org/276383002/diff/80001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/276383002/diff/80001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:431: if
((base::win::IsMetroProcess() &&
This is a copy/paste of the previous check, right?
Can we encapsulate this check in a separate function?

Actually, I think it could be useful to have a member variable in the factory
class, that stores the outcome of this check and then we only check that bool. 
Besides only doing this check once, this will allow us in crash dumps to see
which code path is active.

[mcasas, 2014-05-19 12:59:49]

PTAL.

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/276383002/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_win.cc:198: NOTIMPLEMENTED();
On 2014/05/19 10:51:37, tommi wrote:
> On 2014/05/15 16:03:23, mcasas wrote:
> > On 2014/05/15 15:18:33, tommi wrote:
> > > not implemented?  am I perhaps looking at a strange diff?
> > 
> > It's missing a comment (like in Mac/Linux/Win). 
> > VideoCaptureDeviceFactory::CreateFactory() should create
> > the appropriate platform factory, but until all platforms
> > are landed, the VCDF::Create(), GetDeviceNames()
> > and GetDeviceSupportedFormat() are still referenced.
> > 
> > I'll clean these in the next CL, FWIW.
> 
> Can we #ifdef this so that it's clear on what platforms these are needed?

Discussed offline.

https://codereview.chromium.org/276383002/diff/80001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_factory_win.cc (right):

https://codereview.chromium.org/276383002/diff/80001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:431: if
((base::win::IsMetroProcess() &&
On 2014/05/19 10:51:37, tommi wrote:
> This is a copy/paste of the previous check, right?
> Can we encapsulate this check in a separate function?
> 
> Actually, I think it could be useful to have a member variable in the factory
> class, that stores the outcome of this check and then we only check that bool.

> Besides only doing this check once, this will allow us in crash dumps to see
> which code path is active.

Changed to member bool. It cleans up the code too.

[tommi (sloooow) - chröme, 2014-05-20 12:23:25]

lgtm

[mcasas, 2014-05-20 12:35:32]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-20 12:36:11]

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

[commit-bot: I haz the power, 2014-05-20 13:08:07]

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_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  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...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-20 13:12:52]

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

[commit-bot: I haz the power, 2014-05-20 13:12:53]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  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...)

[mcasas, 2014-05-20 14:23:51]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-20 14:24:26]

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

[mcasas, 2014-05-20 14:42:16]

The CQ bit was unchecked by mcasas@chromium.org

[mcasas, 2014-05-20 18:04:11]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-05-20 18:04:56]

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

[commit-bot: I haz the power, 2014-05-20 20:59:17]

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-21 01:49:23]

Change committed as 271800