Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
546803002:
Win Video Capture: add support for WDM capture devices using a WDM Crossbar filter. (Closed)
|
Created:
6 years, 3 months ago by mcasas Modified:
6 years, 3 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionWin Video Capture: add support for WDM capture devices using a WDM Crossbar filter.
BUG=402684
Committed: https://crrev.com/f7068baee8fbcc33dee9d6d163888f1d8962eb15
Cr-Commit-Position: refs/heads/master@{#294356}
Patch Set 1 #
Total comments: 19
Patch Set 2 : perkj@s comments #
Total comments: 17
Patch Set 3 : perkj@s comments #
Total comments: 1
Patch Set 4 : Simplified CL to only USB capture devices. #
Total comments: 6
Patch Set 5 : perkj@s nits #
Messages
Total messages: 26 (9 generated)
mcasas@chromium.org changed reviewers: + magjed@chromium.org, perkj@chromium.org
perkj@, magjed@ PTAL.
https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:67: if ((exact_name_comparison && device_path.compare(device_id) == 0) || device_path == device_id https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:68: (!exact_name_comparison && (device_path.find(device_id) nit (!exact_name_comparison && (device_path.find(device_id) != std::string::npos)) https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:102: return hr && (!*filter ? E_FAIL : S_OK); This looks dangerous and hard to read. I don't think you should call capture_filter.Detach() if moniker->BindToObject fails right? Can capture_filter.Detach() fail? If so can you keep the old code for generating the error. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:599: // extract the correct pins from each. The necessary pins are device specific This is confusing sentence to read and saying that its expertmental scares me. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:604: device_name_.name(), AM_KSCATEGORY_CROSSBAR, L"FriendlyName", false); Which filter uses the device_name.id() ? Ie, what if you have two capture cards installed? https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:610: DPLOG_IF(ERROR, FAILED(hr)) << "Failed to bind crossbar filter"; nit: can we remove one of this log lines and only log the error case. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:617: crossbar_filter_, PINDIR_OUTPUT, GUID_NULL); GetPin finds the first output pin right? Isn't there a pin type you can check as well? https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:620: DVLOG_IF(1, crossbar_video_output_pin_) dito - please remove one log line. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:642: DVLOG_IF(1, SUCCEEDED(hr)) << "WDM filter instantiated OK."; dito
Patchset #2 (id:20001) has been deleted
PTAL. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:67: if ((exact_name_comparison && device_path.compare(device_id) == 0) || On 2014/09/05 15:04:21, perkj wrote: > device_path == device_id Done. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:68: (!exact_name_comparison && (device_path.find(device_id) On 2014/09/05 15:04:21, perkj wrote: > nit > > (!exact_name_comparison && > (device_path.find(device_id) != std::string::npos)) > Done. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:102: return hr && (!*filter ? E_FAIL : S_OK); On 2014/09/05 15:04:21, perkj wrote: > This looks dangerous and hard to read. I don't think you should call > capture_filter.Detach() if moniker->BindToObject fails right? > > Can capture_filter.Detach() fail? If so can you keep the old code for generating > the error. Reverted to previous code. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:597: nit: remove extra line. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:599: // extract the correct pins from each. The necessary pins are device specific On 2014/09/05 15:04:21, perkj wrote: > This is confusing sentence to read and saying that its expertmental scares me. > Rewritten. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:604: device_name_.name(), AM_KSCATEGORY_CROSSBAR, L"FriendlyName", false); On 2014/09/05 15:04:21, perkj wrote: > Which filter uses the device_name.id() ? Ie, what if you have two capture cards > installed? You're right, here I should search for the exact device.id(), which is unique. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:610: DPLOG_IF(ERROR, FAILED(hr)) << "Failed to bind crossbar filter"; On 2014/09/05 15:04:21, perkj wrote: > nit: can we remove one of this log lines and only log the error case. Done. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:617: crossbar_filter_, PINDIR_OUTPUT, GUID_NULL); On 2014/09/05 15:04:21, perkj wrote: > GetPin finds the first output pin right? Isn't there a pin type you can check as > well? I'm afraid not. I printed out all Crossbar Pin types and they are empty just like for the MJPEG filter[1]; you are most welcome to double check in GraphEdit. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:620: DVLOG_IF(1, crossbar_video_output_pin_) On 2014/09/05 15:04:21, perkj wrote: > dito - please remove one log line. Done. https://codereview.chromium.org/546803002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:642: DVLOG_IF(1, SUCCEEDED(hr)) << "WDM filter instantiated OK."; On 2014/09/05 15:04:21, perkj wrote: > dito Done. (Removed all the positive cases' logging).
Sorry - I ran out of time... https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:80: // device but might also be AM_KSCATEGORY_CAPTURE or AM_KSCATEGORY_CROSSBAR, to nit devices https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:94: moniker = FindDeviceAndReturnMoniker(device_id, device_class_id, It feels wrong look in the Description and friendly name for a device id here. The device _id is supposed to uniquely identify one and only one device even if there are two of the same type and model attached. You also don't seem to use it? InstantiateWDMFiltersAndPins does not call this method. Is it ever used? I see that this is not really new code. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:592: DVLOG_IF(ERROR, FAILED(hr)) << "Anti-flicker setting failed."; Why are you changing the log macros? https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:600: // extract the correct pins from each. The necessary pins are device specific Can you document the pins you are looking for?
A few more. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:78: // Finds and creates a DirectShow Video Capture filter matching the device_name. |device_id| https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:604: device_name_.id(), AM_KSCATEGORY_CROSSBAR, L"DevicePath", false); shouldnt this be an exact id comparison of the DevicePath? https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:628: while (!wdm_source_moniker && t.GetNext()) { Does this mean that you first just look for the vendor and don't use the model name at all and if the vendor match - you use it? What if you have two capture cards of different models from the same vendor? Should you first try to match the whole name first, then maybe the model and last the vendor?
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
PTAL https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:78: // Finds and creates a DirectShow Video Capture filter matching the device_name. On 2014/09/09 07:54:43, perkj wrote: > |device_id| Done. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:80: // device but might also be AM_KSCATEGORY_CAPTURE or AM_KSCATEGORY_CROSSBAR, to On 2014/09/08 13:32:11, perkj wrote: > nit devices Done. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:94: moniker = FindDeviceAndReturnMoniker(device_id, device_class_id, On 2014/09/08 13:32:11, perkj wrote: > It feels wrong look in the Description and friendly name for a device id here. > The device _id is supposed to uniquely identify one and only one device even if > there are two of the same type and model attached. This loop looks for a device by DevicePath, then Description, then FriendlyName, whichever is available first, and is a provision mainly for non-USB devices that have _no_ DevicePath (the Registry style name). For instance, true fact: BlackMagic DeckLink cards and BlueFish 555s do not have a DevicePath, but have FriendlyName. How do you identify which card you want to use if you have more than one of these in your system? You don't, because you can't identify which one via DirectShow API. This is discussed over internet and in their Forums, IIRC. > You also don't seem to use it? InstantiateWDMFiltersAndPins does not call this > method. It was factored out to be used in l.603 and l.629 of the new code. > > Is it ever used? Yes > I see that this is not really new code. It isn't, is refactored. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:592: DVLOG_IF(ERROR, FAILED(hr)) << "Anti-flicker setting failed."; On 2014/09/08 13:32:11, perkj wrote: > Why are you changing the log macros? Not intentional, reverted. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:600: // extract the correct pins from each. The necessary pins are device specific On 2014/09/08 13:32:11, perkj wrote: > Can you document the pins you are looking for? Done. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:604: device_name_.id(), AM_KSCATEGORY_CROSSBAR, L"DevicePath", false); On 2014/09/09 07:54:43, perkj wrote: > shouldnt this be an exact id comparison of the DevicePath? Done. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:628: while (!wdm_source_moniker && t.GetNext()) { On 2014/09/09 07:54:43, perkj wrote: > Does this mean that you first just look for the vendor and don't use the model > name at all and if the vendor match - you use it? What if you have two capture > cards of different models from the same vendor? > Should you first try to match the whole name first, then maybe the model and > last the vendor? This is a fuzzy matching so it isn't perfect. We don't really know exactly where the model and/or vendor substrings are, e.g. some cards are called "WDM Card Crossbar", others are simply called "Vendor Card Crossbar". Can you propose any alternative?
https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:94: moniker = FindDeviceAndReturnMoniker(device_id, device_class_id, On 2014/09/09 08:22:26, mcasas wrote: > On 2014/09/08 13:32:11, perkj wrote: > > It feels wrong look in the Description and friendly name for a device id here. > > The device _id is supposed to uniquely identify one and only one device even > if > > there are two of the same type and model attached. > > This loop looks for a device by DevicePath, then Description, > then FriendlyName, whichever is available first, and is a > provision mainly for non-USB devices that have _no_ DevicePath > (the Registry style name). For instance, true fact: BlackMagic > DeckLink cards and BlueFish 555s do not have a DevicePath, but > have FriendlyName. > > How do you identify which card you want to use if you have more > than one of these in your system? You don't, because you can't > identify which one via DirectShow API. This is discussed over > internet and in their Forums, IIRC. > > > You also don't seem to use it? InstantiateWDMFiltersAndPins does not call this > > method. > > It was factored out to be used in l.603 and l.629 of the new code. > > > > > Is it ever used? > Yes > > > I see that this is not really new code. > > It isn't, is refactored. > > Acknowledged. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:628: while (!wdm_source_moniker && t.GetNext()) { On 2014/09/09 08:22:26, mcasas wrote: > On 2014/09/09 07:54:43, perkj wrote: > > Does this mean that you first just look for the vendor and don't use the model > > name at all and if the vendor match - you use it? What if you have two capture > > cards of different models from the same vendor? > > Should you first try to match the whole name first, then maybe the model and > > last the vendor? > > This is a fuzzy matching so it isn't perfect. We don't really > know exactly where the model and/or vendor substrings are, > e.g. some cards are called "WDM Card Crossbar", others are > simply called "Vendor Card Crossbar". Can you propose any > alternative? > For your example- wouldn't "WDM Card Crossbar" make t.GetNext() == WDM. So you only match on WDM. Don't you need to use more than just one token?
lgtm https://codereview.chromium.org/546803002/diff/100001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/100001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:246: if(crossbar_filter_) nit: should have space between if and '('
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
perkj@ PTAL. CL has been greatly simplified with limiting the scope to USB devices. I recommend comparing the last patch to the baseline, not to the previous patch. https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_win.cc:628: while (!wdm_source_moniker && t.GetNext()) { On 2014/09/09 10:51:08, perkj wrote: > On 2014/09/09 08:22:26, mcasas wrote: > > On 2014/09/09 07:54:43, perkj wrote: > > > Does this mean that you first just look for the vendor and don't use the > model > > > name at all and if the vendor match - you use it? What if you have two > capture > > > cards of different models from the same vendor? > > > Should you first try to match the whole name first, then maybe the model and > > > last the vendor? > > > > This is a fuzzy matching so it isn't perfect. We don't really > > know exactly where the model and/or vendor substrings are, > > e.g. some cards are called "WDM Card Crossbar", others are > > simply called "Vendor Card Crossbar". Can you propose any > > alternative? > > > > For your example- wouldn't "WDM Card Crossbar" make t.GetNext() == WDM. So you > only match on WDM. Don't you need to use more than just one token? > OK after some offline discussion, we have decided to go for USB only capture cards at this stage, so we can use full matching of USB VID+PID. This is actually even better, because it allows for using device_id_.capabilities_id(), IOW the associated WDM Capture Filter ID is known from the factory enumeration. Moreover, I can remove FindDeviceAndReturnMoniker() and leave GetDeviceFilter() almost as it was before. For the record, the EzCap digitiser needs the filters: - WDM 2861 Crossbar - USB 2861 Device And the Hauppauge in the bug must use: - Hauppauge Cx23100 Crossbar - Hauppauge Cx23100 Video Capture
lgtm
lgtm https://codereview.chromium.org/546803002/diff/130004/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/130004/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:25: // static nit. Remove static https://codereview.chromium.org/546803002/diff/130004/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:44: // static dito https://codereview.chromium.org/546803002/diff/130004/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:50: return connection_media_type.majortype == major_type; suggest return SUCCEEDED(hr) && connection_media_type.majortype == major_type;
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/546803002/170001
https://codereview.chromium.org/546803002/diff/130004/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/546803002/diff/130004/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:25: // static On 2014/09/10 14:58:26, perkj wrote: > nit. Remove static Done. https://codereview.chromium.org/546803002/diff/130004/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:44: // static On 2014/09/10 14:58:26, perkj wrote: > dito Done. https://codereview.chromium.org/546803002/diff/130004/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:50: return connection_media_type.majortype == major_type; On 2014/09/10 14:58:26, perkj wrote: > suggest return SUCCEEDED(hr) && connection_media_type.majortype == major_type; Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (None)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/546803002/170001
Message was sent while issue was closed.
Committed patchset #5 (id:170001) as f9b4416e1fd3c011d5b84bb701d1b5ba2b30d1ac
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f7068baee8fbcc33dee9d6d163888f1d8962eb15 Cr-Commit-Position: refs/heads/master@{#294356} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
