Reconnect support for DirectShow video capture devices in parallel to MediaFoundation ones.

Reconnect support for DirectShow video capture devices in parallel to MediaFoundation ones.


Implemented merging device driver type for capture devices either Media Foundation
 or DirectShow. Added a static (but local namespace) map indexed by unique id 
 having the appropriate type. This is used in Create() to, well, create the 
 appropriate driver fron unique_id. 

Merged MF and DS devices lists, for this added operator== and operator< to 
 VideoCaptureDevice struct; also accepted UYVY and ARGB colorspaces from video 
 capture device caps list.

BUG=https://code.google.com/p/chromium/issues/detail?id=144465

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

[mcasas, 2013-06-18 15:23:01]

Hi Magnus,
a heads up: I'm trying to get an all-green trybot set for this patch that would
allow to use DirectShow for "other" video capture devices (read: non USB) - the
"good" video devices are manipulated via MediaFoundation. At the moment those
two are mutually exclusive...
Cheers!
Miguel

[tommi (sloooow) - chröme, 2013-06-24 11:09:27]

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/video_...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/video_...
media/video/capture/video_capture_device.h:35: friend bool operator==(const
Name& left, const Name& right) {
In general operator overloading is discouraged but I'm assuming this is needed
for some STL feature?  Can you add comments that explains why we don't implement
an IsEqual/Equal method instead (which is what the style guide suggests).

Btw, why do you need the friend qualifier?

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/video_...
media/video/capture/video_capture_device.h:39: return (this->unique_id <
other.unique_id);
remove "this->"

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:21: kDirectShow,  //
DirectShow driver type
Some style issues but since we're switching on the API used and not necessarily
the device driver type (they both support UVC, right?), what about something
like:

enum CaptureApiType {
  DIRECT_SHOW,
  MEDIA_FOUNDATION,
};

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:26: DeviceDriverTypeMap
device_driver_type_;
sorry, no global variables that require construction.  This impacts Chrome's
startup cost and the cl would be reverted due to breaking the performance bots
(we actually keep count of how many global variables we allow, so adding one
more breaks the build).

Btw, naming of global variables differ from naming of member variables.  should
be g_device_driver_type.

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:27: 
nit: one empty line

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:167:
device_driver_type_[it->unique_id] = kMF;
instead of the global map, what do you think about moving the enum into the
Names structure (it's ok to make it #ifdef'ed for Windows) and store the API
type as a member of that structure?
I think that should make this a bit simpler and avoid having to do any operator
overloading etc.

[mcasas, 2013-06-25 07:04:36]

Addressed your comments as per our conversations. No global map but retrieves
list of MF (and DS) devices both on GetDeviceNames() and on Create().

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/video_...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/video_...
media/video/capture/video_capture_device.h:35: friend bool operator==(const
Name& left, const Name& right) {
On 2013/06/24 11:09:27, tommi wrote:
> In general operator overloading is discouraged but I'm assuming this is needed
> for some STL feature?  Can you add comments that explains why we don't
implement
> an IsEqual/Equal method instead (which is what the style guide suggests).
> 
Ok, supposition correct, done.
> Btw, why do you need the friend qualifier?
Actually no, I just put it out of routine.

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/video_...
media/video/capture/video_capture_device.h:39: return (this->unique_id <
other.unique_id);
On 2013/06/24 11:09:27, tommi wrote:
> remove "this->"

Done.

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:21: kDirectShow,  //
DirectShow driver type
On 2013/06/24 11:09:27, tommi wrote:
> Some style issues but since we're switching on the API used and not
necessarily
> the device driver type (they both support UVC, right?), what about something
> like:
> 
> enum CaptureApiType {
>   DIRECT_SHOW,
>   MEDIA_FOUNDATION,
> };

Done.

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:26: DeviceDriverTypeMap
device_driver_type_;
On 2013/06/24 11:09:27, tommi wrote:
> sorry, no global variables that require construction.  This impacts Chrome's
> startup cost and the cl would be reverted due to breaking the performance bots
> (we actually keep count of how many global variables we allow, so adding one
> more breaks the build).
> 
> Btw, naming of global variables differ from naming of member variables. 
should
> be g_device_driver_type.

All right, quote our conversation later on, it should be gone.

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:27: 
On 2013/06/24 11:09:27, tommi wrote:
> nit: one empty line

Done.

https://codereview.chromium.org/17402002/diff/3001/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:167:
device_driver_type_[it->unique_id] = kMF;
On 2013/06/24 11:09:27, tommi wrote:
> instead of the global map, what do you think about moving the enum into the
> Names structure (it's ok to make it #ifdef'ed for Windows) and store the API
> type as a member of that structure?
> I think that should make this a bit simpler and avoid having to do any
operator
> overloading etc.

I added the Capture Api type to VideoCaptureDevice::Name, for OS_WIN compilation
only. However, the gap between GetDeviceNames() and Create() cannot be easily
jumped since VideoCaptureManager,     "parent" of VideoCaptureDeviceWin, cherry
picks attributes from the mentioned struct Name, instead of copying it all, and
stores them into a StreamDeviceInfo Array, and it would get very ugly if OS_WIN
parts would proliferate...

[tommi (sloooow) - chröme, 2013-06-26 09:53:21]

FYI - I started a new CL to make adding the enum to the Name struct possible:
https://codereview.chromium.org/17846002/

[tommi (sloooow) - chröme, 2013-06-26 09:57:27]

https://codereview.chromium.org/17402002/diff/36002/media/video/capture/win/v...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/17402002/diff/36002/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:175: Names::iterator it;
you shouldn't need to do any enumeration or searching here since the
|capture_api_type| member variable should tell you which API to use.

[mcasas, 2013-06-27 15:47:53]

following cl 17846002

[mcasas, 2013-07-02 12:28:11]

Rebaselined with some difficulty and cleaned up.

https://codereview.chromium.org/17402002/diff/36002/media/video/capture/win/v...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/17402002/diff/36002/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:175: Names::iterator it;
On 2013/06/26 09:57:27, tommi wrote:
> you shouldn't need to do any enumeration or searching here since the
> |capture_api_type| member variable should tell you which API to use.

Done.

[mcasas, 2013-07-03 09:40:08]

We seem to be ready again for review :)

[tommi (sloooow) - chröme, 2013-07-03 12:06:51]

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
media/video/capture/video_capture_device.h:62: };
nit: empty line after this one

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
media/video/capture/video_capture_device.h:64: const CaptureApiType&
capture_api_type() const { return capture_api_type_; }
return by value

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
media/video/capture/video_capture_device.h:65: void capture_api_type(const
CaptureApiType & type) {
set_capture_api_type(CaptureApiType type)

note the "set_" prefix and we pass enum types by value like we do with ints.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
media/video/capture/video_capture_device.h:74: CaptureApiType capture_api_type_;
right now this variable isn't initialized in the constructor.  Let's initialize
it to UNKNOWN_CAPTURE_API (or something like that) in both constructors and add
a DCHECK to capture_api_type():

CaptureApiType capture_api_type() const {
  DCHECK_NE(capture_api_type_, UNKNOWN_CAPTURE_API);
  return capture_api_type_;
}

Actually, what about removing the capture_api_type set method and instead add a
Windows-only constructor that accepts 3 arguments (adding the api selector)? 
The point is that we should be setting the API enum variable only when we first
construct the Name instance inside the respective implementation.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/win/v...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:157:
it->capture_api_type(Name::MEDIA_FOUNDATION);
do this instead inside VideoCaptureDeviceMFWin::GetDeviceNames and
VideoCaptureDeviceWin::GetDeviceNames respectively.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:190: DLOG(ERROR) << "
Couldn't recognize VideoCaptureDevice type";
NOTREACHED()?

[mcasas, 2013-07-03 12:41:10]

Addressed comments, new patch coming.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
media/video/capture/video_capture_device.h:62: };
On 2013/07/03 12:06:51, tommi wrote:
> nit: empty line after this one

Done.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
media/video/capture/video_capture_device.h:64: const CaptureApiType&
capture_api_type() const { return capture_api_type_; }
On 2013/07/03 12:06:51, tommi wrote:
> return by value

Done.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
media/video/capture/video_capture_device.h:65: void capture_api_type(const
CaptureApiType & type) {
On 2013/07/03 12:06:51, tommi wrote:
> set_capture_api_type(CaptureApiType type)
> 
> note the "set_" prefix and we pass enum types by value like we do with ints.

Done.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/video...
media/video/capture/video_capture_device.h:74: CaptureApiType capture_api_type_;
The second sounds better to me.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/win/v...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:157:
it->capture_api_type(Name::MEDIA_FOUNDATION);
I agree. Wasn't sure on that one, if it was best to write this ApiType assigment
inside the individual GetDeviceNames or put them here, but now you shifted the
scales. Done.

https://codereview.chromium.org/17402002/diff/71001/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:190: DLOG(ERROR) << "
Couldn't recognize VideoCaptureDevice type";
On 2013/07/03 12:06:51, tommi wrote:
> NOTREACHED()?

Done.

[mcasas, 2013-07-03 13:18:40]

Refactored a bit more Names::CaptureApiType, I added a class to wrap the type, 
so the type would have a by default UNKNOWN type, hence protecting a future 
user not using the constructor with Type assignment.

[tommi (sloooow) - chröme, 2013-07-03 15:27:26]

lgtm with a few minor requests

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/video...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/video...
media/video/capture/video_capture_device.h:91: DCHECK(capture_api_type_ != 
API_TYPE_UNKNOWN);
nit: DCHECK_NE

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/win/v...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:171:
VideoCaptureDeviceMFWin::PlatformSupported()) {
the second check shouldn't be necessary anymore so you could change this to be:

if (device_name.capture_api_type() == Name::MEDIA_FOUNDATION) {
  DCHECK(VideoCaptureDeviceMFWin::PlatformSupported());
  ...

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:180: DLOG(INFO) << "
DirectShow Device: " << device_name.name();
Change all DLOG(INFO)s to DVLOG(1).
(we don't use DLOG(INFO) anymore)

[mcasas, 2013-07-04 08:58:15]

Needed small adaptation of VideoCaptureDeviceTest, to take into account that in
Windows platforms, creation of devices need a CaptureApiType, otherwise a DCHECK
will jump. Also, a check for framerate == 30 was removed, since this depends on
the underlying video capture, in my case a USB camera giving something else

[mcasas, 2013-07-04 10:40:42]

Comments addressed and unittests corrected.

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/video...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/video...
media/video/capture/video_capture_device.h:91: DCHECK(capture_api_type_ != 
API_TYPE_UNKNOWN);
On 2013/07/03 15:27:26, tommi wrote:
> nit: DCHECK_NE

Done.

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/win/v...
File media/video/capture/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:171:
VideoCaptureDeviceMFWin::PlatformSupported()) {
On 2013/07/03 15:27:26, tommi wrote:
> the second check shouldn't be necessary anymore so you could change this to
be:
> 
> if (device_name.capture_api_type() == Name::MEDIA_FOUNDATION) {
>   DCHECK(VideoCaptureDeviceMFWin::PlatformSupported());
>   ...

Done.

https://codereview.chromium.org/17402002/diff/80001/media/video/capture/win/v...
media/video/capture/win/video_capture_device_win.cc:180: DLOG(INFO) << "
DirectShow Device: " << device_name.name();
On 2013/07/03 15:27:26, tommi wrote:
> Change all DLOG(INFO)s to DVLOG(1).
> (we don't use DLOG(INFO) anymore)

Done.

[tommi (sloooow) - chröme, 2013-07-04 12:43:25]

lgtm

[commit-bot: I haz the power, 2013-07-04 18:52:08]

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

[commit-bot: I haz the power, 2013-07-04 20:08:37]

Retried try job too often on linux_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-07-05 08:21:29]

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

[commit-bot: I haz the power, 2013-07-05 08:35:58]

Change committed as 210280

[mcasas, 2013-07-05 11:43:00]

After XP test VideoCaptureDeviceTest.OpenInvalidDevice force patch reverted

[mcasas, 2013-07-05 11:45:45]

Ooops by default the patch was added to the original CL... Anyway the change is
small: I added a check for Windows version in
VideoCaptureDeviceTest.OpenInvalidDevice, Capture Driver Api is DS if Windows
version < 7 and MF otherwise. This made a DCHECK fail for XP.