Win Video Capture: add DirectShow WDM devices capabilities enumeration.

Win Video Capture: add DirectShow WDM devices capabilities enumeration.

BUG=402684
Committed: https://crrev.com/037a83ca04d880a4381192df432c141a398dd668
Cr-Commit-Position: refs/heads/master@{#293127}

[mcasas, 2014-09-01 09:48:24]

mcasas@chromium.org changed reviewers:
+ magjed@chromium.org, perkj@chromium.org

[mcasas, 2014-09-01 09:48:25]

perkj@, magjed@ PTAL.

[perkj_chrome, 2014-09-01 09:57:43]

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

https://codereview.chromium.org/517273004/diff/20001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:241: hr =
VideoCaptureDeviceWin::GetDeviceFilter(*iter,
GetDeviceFilter need the correct Name::id. So why is this necessary? Why not
make sure that |device| have the correct id from the beginning when the name is
enumerated?

[mcasas, 2014-09-01 15:45:57]

perkj@ code modified according to offline conversation, PTAL!

[perkj_chrome, 2014-09-02 09:22:04]

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/vide...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/vide...
media/video/capture/video_capture_device.h:110: const std::string&
capabilities_unique_id() const {
add comment what this id really is and why this is not the same as id().
Should this maybe be names capabilities_id()?

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

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:109: {
AM_KSCATEGORY_CROSSBAR, Name::DIRECT_SHOW_WDM}  // Must be the last entry.
http://msdn.microsoft.com/en-us/library/windows/desktop/dd373520(v=vs.85).aspx
suggest that there are other types of WDM types as well and that
AM_KSCATEGORY_CROSSBAR is just for analog video input. 

Should Name::DIRECT_SHOW_WDM be changed to NAME::DIRECT_SHOW_AMCROSSBAR ? 

http://msdn.microsoft.com/en-us/library/windows/desktop/dd388914(v=vs.85).aspx

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:112: for (int
class_index = 0; class_index < arraysize(kDirectShowFilterClasses);
instead of a for loop- would it make sence to put line 113 to 166 in a separate
method?

static void GetDeviceNamesDirectShow(xxx class_id, Names* device_names);

Then GetDeviceNamesDirectShow(Names* device_names)
Could call 
 {
GetDeviceNamesDirectShow(CLSID_VideoInputDeviceCategory, device_names) 
GetDeviceNamesDirectShow(AM_KSCATEGORY_CROSSBAR, device_names) 
... Special handle the AM_KSCATEGORY_CROSSBAR.
That would simplify reading the code a bit and clarify why the order matters.

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:170: // find a
device with matching USB ID.
you are comparing GetModel()  to find the USB id of the first device - of that
model.

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:175: if
(device_model.compare(iter->GetModel()) == 0) {
prefer ==

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:176:
device.set_capabilities_unique_id(iter->id());
Do you know what happens if you have two capture cards of the same model? Would
they have the same set of capabilities?

[mcasas, 2014-09-02 15:13:42]

perkj@ PTAL.

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/vide...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/vide...
media/video/capture/video_capture_device.h:110: const std::string&
capabilities_unique_id() const {
On 2014/09/02 09:22:03, perkj wrote:
> add comment what this id really is and why this is not the same as id().
> Should this maybe be names capabilities_id()?

Done.

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

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:109: {
AM_KSCATEGORY_CROSSBAR, Name::DIRECT_SHOW_WDM}  // Must be the last entry.
On 2014/09/02 09:22:03, perkj wrote:
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd373520(v=vs.85).aspx
> suggest that there are other types of WDM types as well and that
> AM_KSCATEGORY_CROSSBAR is just for analog video input. 
> 
> Should Name::DIRECT_SHOW_WDM be changed to NAME::DIRECT_SHOW_AMCROSSBAR ? 
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd388914(v=vs.85).aspx

Done.

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:112: for (int
class_index = 0; class_index < arraysize(kDirectShowFilterClasses);
On 2014/09/02 09:22:03, perkj wrote:
> instead of a for loop- would it make sence to put line 113 to 166 in a
separate
> method?
> 
> static void GetDeviceNamesDirectShow(xxx class_id, Names* device_names);
> 
> Then GetDeviceNamesDirectShow(Names* device_names)
> Could call 
>  {
> GetDeviceNamesDirectShow(CLSID_VideoInputDeviceCategory, device_names) 
> GetDeviceNamesDirectShow(AM_KSCATEGORY_CROSSBAR, device_names) 
> ... Special handle the AM_KSCATEGORY_CROSSBAR.
> That would simplify reading the code a bit and clarify why the order matters. 
> 
> 

Done.

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:170: // find a
device with matching USB ID.
On 2014/09/02 09:22:03, perkj wrote:
> you are comparing GetModel()  to find the USB id of the first device - of that
> model. 

Clarified.

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:175: if
(device_model.compare(iter->GetModel()) == 0) {
On 2014/09/02 09:22:03, perkj wrote:
> prefer == 

Not sure I understand. Can you please clarify?

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:176:
device.set_capabilities_unique_id(iter->id());
On 2014/09/02 09:22:03, perkj wrote:
> Do you know what happens if you have two capture cards of the same model?
Would
> they have the same set of capabilities?

Yes.

[magjed_chromium, 2014-09-02 15:14:41]

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

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:153: if
((wcsstr(str_ptr, L"(VFW)") == NULL) &&
These parentheses are wrong. && has higher precedence, so the current condition
is actually:
(wcsstr(str_ptr, L"(VFW)") == NULL && lstrlenW(str_ptr) < name_length) ||
LowerCaseEqualsASCII(...)

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:158: std::string
device_name(base::SysWideToUTF8(str_ptr));
device_name can be made const

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:167: Name
device(device_name, id,
device can be made const

https://codereview.chromium.org/517273004/diff/60001/media/video/capture/win/...
media/video/capture/win/video_capture_device_factory_win.cc:244: DVLOG(2) <<
"Failed to create capture filter." ;
Unnecessary space after string :)

[perkj_chrome, 2014-09-03 08:42:41]

nice. Much more readable I think.

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

https://codereview.chromium.org/517273004/diff/120001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:410:
crossbar_device_it != crossbar_device_names.end();
indentation

https://codereview.chromium.org/517273004/diff/120001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:414: device_it !=
device_names->end(); ++device_it) {
indentation

https://codereview.chromium.org/517273004/diff/120001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:415: if
(crossbar_device_model.compare(device_it->GetModel()) == 0) {
prefer if crossbar_device_model == device_it->GetModel()

[mcasas, 2014-09-03 09:51:06]

perkj@ PTAL

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

https://codereview.chromium.org/517273004/diff/120001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:410:
crossbar_device_it != crossbar_device_names.end();
On 2014/09/03 08:42:41, perkj wrote:
> indentation

Done.

https://codereview.chromium.org/517273004/diff/120001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:414: device_it !=
device_names->end(); ++device_it) {
On 2014/09/03 08:42:41, perkj wrote:
> indentation

Done.

https://codereview.chromium.org/517273004/diff/120001/media/video/capture/win...
media/video/capture/win/video_capture_device_factory_win.cc:415: if
(crossbar_device_model.compare(device_it->GetModel()) == 0) {
On 2014/09/03 08:42:41, perkj wrote:
> prefer if crossbar_device_model == device_it->GetModel()

Done.

[perkj_chrome, 2014-09-03 11:15:02]

lgtm

[magjed_chromium, 2014-09-03 11:27:24]

lgtm

I will fix the broken if-statement in a separate CL as it is not related to this
CL.

[mcasas, 2014-09-03 11:37:01]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-09-03 11:38:01]

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

[commit-bot: I haz the power, 2014-09-03 12:38:45]

Committed patchset #5 (id:160001) as 649119ff34727badd9d0a404a283ae570216f7dd

[commit-bot: I haz the power, 2014-09-10 03:25:27]

Patchset 5 (id:??) landed as
https://crrev.com/037a83ca04d880a4381192df432c141a398dd668
Cr-Commit-Position: refs/heads/master@{#293127}