Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1039383002:
Video Capture Win: Adds support for Analog Capture Devices such as the USB2-Live and Avermedia Card. (Closed)
|
Created:
5 years, 9 months ago by ken.strickland Modified:
5 years, 8 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for the USB2-Live and Avermedia Analog capture devices by constructing a CLSID_CaptureGraphBuilder2, and then using the FindInterface(IID_IAMStreamConfig) that will add the needed IAMCrossbar to complete the DS graph appropriately.
Removed the ability for the user to explicitly choose AM_KSCATEGORY_CROSSBAR devices as it is unnecessary.
BUG=402684
TEST=Attach a USB2-Live device, navigate to;
https://src.chromium.org/svn/trunk/src/chrome/test/data/webrtc/manual/peerconnection.html
click, "Get Devices", choose the "USB2-Live", then click on "Request GetUserMedia"
Committed: https://crrev.com/773aa4541957d8ad7d28990005544e98ee534474
Cr-Commit-Position: refs/heads/master@{#323831}
Patch Set 1 #
Total comments: 37
Patch Set 2 #
Total comments: 12
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 5
Patch Set 5 : #
Total comments: 2
Patch Set 6 : Rebase #Patch Set 7 : Rebase #
Messages
Total messages: 37 (16 generated)
ken.strickland@gmail.com changed reviewers: + mcasas@chromium.org, perkj@chromium.org
Fix for Issue 402684 is ready for review, let me know how I can help.
Took the liberty of launching a few Win bots. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_factory_win.cc:131: const CLSID& class_id, This parameter is now always CLSID_VideoInputDeviceCategory in all function calls, so there's no need for it. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_factory_win.cc:132: const Name::CaptureApiType capture_api_type, Similar thing here, |capture_api_type| is always Name::DIRECT_SHOW. Can (and should) be removed. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_factory_win.cc:426: device_names); If you're removing support for WDM devices explicitly, then remove its associated Capture Type [1]. You'll need to update media_internals unit test [2]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:181: media_type_= NULL; Revert this change. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:241: video_stream_config_.Release(); No tabs. Two space indent after curly bracket. No need for curly brackets for one-line blocks. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:245: graph_builder2_.Release(); Same here, and in several other parts below. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.html and https://www.chromium.org/developers/coding-style https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:253: hr = graph_builder2_.CreateInstance(CLSID_CaptureGraphBuilder2, NULL, In the spirit of moving decl/ctors as close as possible to where they are used, please move this to the vicinity of l.322. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:322: // The following FindInterface calls are to build the upstream portions of the graph Lines are max 80 chars. Please redo argument formatting. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:323: hr = graph_builder2_->FindInterface(&PIN_CATEGORY_CAPTURE, &PIN_CATEGORY_CAPTURE looks like "address of" a constant/enum...? :? Same comment in the next line and l.327,328. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:331: DLOG(ERROR) << "Failed to find CapFilter:IAMStreamConfig: " DLOG_IF(ERROR, FAILED(hr)) << "..." https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:384: if (media_type->formattype == FORMAT_VideoInfo) { In this code, as opposed to before, we would continue doing stuff even after capabilities' retrieval has failed on the capture filter. Is that the expected/correct behaviour? https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:391: sink_filter_->SetRequestedMediaFormat(format); I can only see [1] as the method signature and it needs three params, perhaps this CL is missing some delta? [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:401: if (format.pixel_format == PIXEL_FORMAT_MJPEG && !mjpg_filter_.get()) { Actually if the device is providing MJPEG, I think it's best to swallow it in the |sink_filter_|. It will be converted to I420 in VideoCaptureDeviceClient. MJPEG is correctly listed in [1] but missing an entry in [2]. BTW the code regarding mjpeg filter addition looks familiar, was it present in a similar way in older revisions of the code...? [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... [2] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:571: capabilities_.emplace_back(stream_index, format); What happened to |h->bmiHeader|? https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_win.h (right): https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.h:23: #include "media/video/capture/video_capture_types.h" You might have your repo out of sink. |video_capture_types.h| moved a few weeks ago to media/base. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.h:99: base::win::ScopedComPtr<ICaptureGraphBuilder2> graph_builder2_; Needs a more meaningful name than |graph_builder2_|. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.h:108: base::win::ScopedComPtr<IPin> output_mjpg_pin_; IIUC, the trick to get HaupPauge to work is to accept MJPEG in a specific Filter. Like I say somewhere down in the definition file, the decoding itself can be (better) done further down the capture path [1], so what we have to do is to configure SinkInputPin to accept that format. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.h:110: base::win::ScopedComPtr<IAMStreamConfig> video_stream_config_; This is only used for Receive()ing on a FindInterface() so it can be made demoted to local variable.
I had to do a bit of a code merge over the weekend, since my repo was a bit outdated. I did those and resubmitted a change set. It looks like there were changes completed with the device capabilities that was not in my repo. My changes to the ds graph still work great, but there is a green bar now on the bottom of my video output while testing the USB2-Live. I'm assuming this is because of the changes to the device capabilities. But this green bar is only there for the USB2-Live and Avermedia, my web cam looks and works fine. Looking over now why this is happening. Like I said this green bar at the bottom seems to be only for the USB2-Live and Avermedia cards. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_factory_win.cc:131: const CLSID& class_id, On 2015/03/27 23:06:55, mcasas wrote: > This parameter is now always CLSID_VideoInputDeviceCategory > in all function calls, so there's no need for it. Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_factory_win.cc:132: const Name::CaptureApiType capture_api_type, On 2015/03/27 23:06:55, mcasas wrote: > Similar thing here, |capture_api_type| is always > Name::DIRECT_SHOW. Can (and should) be removed. Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_factory_win.cc:426: device_names); On 2015/03/27 23:06:55, mcasas wrote: > If you're removing support for WDM devices explicitly, then remove its > associated Capture Type [1]. You'll need to update media_internals > unit test [2]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_factory_win.cc:426: device_names); On 2015/03/27 23:06:55, mcasas wrote: > If you're removing support for WDM devices explicitly, then remove its > associated Capture Type [1]. You'll need to update media_internals > unit test [2]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:181: media_type_= NULL; On 2015/03/27 23:06:55, mcasas wrote: > Revert this change. Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:241: video_stream_config_.Release(); On 2015/03/27 23:06:55, mcasas wrote: > No tabs. Two space indent after curly bracket. No need for > curly brackets for one-line blocks. Partly was my repo being out of sync, and I did some fixes you mentioned. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:245: graph_builder2_.Release(); On 2015/03/27 23:06:55, mcasas wrote: > Same here, and in several other parts below. > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html > and > https://www.chromium.org/developers/coding-style Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:253: hr = graph_builder2_.CreateInstance(CLSID_CaptureGraphBuilder2, NULL, On 2015/03/27 23:06:55, mcasas wrote: > In the spirit of moving decl/ctors as close as possible to where > they are used, please move this to the vicinity of l.322. Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:322: // The following FindInterface calls are to build the upstream portions of the graph On 2015/03/27 23:06:55, mcasas wrote: > Lines are max 80 chars. Please redo argument formatting. Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:323: hr = graph_builder2_->FindInterface(&PIN_CATEGORY_CAPTURE, On 2015/03/27 23:06:55, mcasas wrote: > &PIN_CATEGORY_CAPTURE looks like "address of" a constant/enum...? :? > > Same comment in the next line and l.327,328. This is correct, looked this up in MSDN https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:331: DLOG(ERROR) << "Failed to find CapFilter:IAMStreamConfig: " On 2015/03/27 23:06:55, mcasas wrote: > DLOG_IF(ERROR, FAILED(hr)) << "..." Done. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:384: if (media_type->formattype == FORMAT_VideoInfo) { On 2015/03/27 23:06:55, mcasas wrote: > In this code, as opposed to before, we would continue doing stuff > even after capabilities' retrieval has failed on the capture filter. Is > that the expected/correct behaviour? Repo out of sync, fixed https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:391: sink_filter_->SetRequestedMediaFormat(format); On 2015/03/27 23:06:55, mcasas wrote: > I can only see [1] as the method signature and it needs > three params, perhaps this CL is missing some delta? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... Repo out of sync, fixed https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:401: if (format.pixel_format == PIXEL_FORMAT_MJPEG && !mjpg_filter_.get()) { On 2015/03/27 23:06:55, mcasas wrote: > Actually if the device is providing MJPEG, I think it's best to swallow > it in the |sink_filter_|. It will be converted to I420 in > VideoCaptureDeviceClient. MJPEG is correctly listed in [1] but > missing an entry in [2]. > > BTW the code regarding mjpeg filter addition looks familiar, was it > present in a similar way in older revisions of the code...? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... Repo out of sync, fixed https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:571: capabilities_.emplace_back(stream_index, format); On 2015/03/27 23:06:55, mcasas wrote: > What happened to |h->bmiHeader|? Repo out of sync, fixed https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_win.h (right): https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.h:23: #include "media/video/capture/video_capture_types.h" On 2015/03/27 23:06:55, mcasas wrote: > You might have your repo out of sink. |video_capture_types.h| moved > a few weeks ago to media/base. Repo was out of sync, fixed. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.h:99: base::win::ScopedComPtr<ICaptureGraphBuilder2> graph_builder2_; On 2015/03/27 23:06:55, mcasas wrote: > Needs a more meaningful name than |graph_builder2_|. I hope capture_graph_builder is ok, not sure what else to call it. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.h:108: base::win::ScopedComPtr<IPin> output_mjpg_pin_; On 2015/03/27 23:06:55, mcasas wrote: > IIUC, the trick to get HaupPauge to work is to accept MJPEG in > a specific Filter. Like I say somewhere down in the definition file, > the decoding itself can be (better) done further down the capture > path [1], so what we have to do is to configure SinkInputPin to > accept that format. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... Repo was out of sync, removing this.. https://codereview.chromium.org/1039383002/diff/1/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.h:110: base::win::ScopedComPtr<IAMStreamConfig> video_stream_config_; On 2015/03/27 23:06:55, mcasas wrote: > This is only used for Receive()ing on a FindInterface() so > it can be made demoted to local variable. Done. https://codereview.chromium.org/1039383002/diff/20001/media/video/capture/win... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/1039383002/diff/20001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:141: hr = dev_enum->CreateClassEnumerator(CLSID_VideoInputDeviceCategory, enum_moniker.Receive(), 0); Fixed over 80 characters https://codereview.chromium.org/1039383002/diff/20001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:165: const std::string device_name(base::SysWideToUTF8(V_BSTR(name.ptr()))); Fixed Tabs https://codereview.chromium.org/1039383002/diff/20001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:176: id = base::SysWideToUTF8(V_BSTR(name.ptr())); Fixed Tabs https://codereview.chromium.org/1039383002/diff/20001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/20001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:96: std::string device_path(base::SysWideToUTF8(V_BSTR(name.ptr()))); Fixed Tabs https://codereview.chromium.org/1039383002/diff/20001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:237: capture_graph_builder.Release(); Fixed Tabs https://codereview.chromium.org/1039383002/diff/20001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:535: capabilities_.emplace_back(stream_index, format, h->bmiHeader); Fixed Tabs
Looking better. Please update description: - no need to mention "Fix for..." in the Title, is understood. - Instead, mention that this is "Video Capture Win:..." so it's easier for Q&A and sheriff to classify. - Give a technical description after mentioning the support that this CL is adding, i.e., "...adds construction of an ICaptureGraphBuilder2 [ref] for these rebel video digitisers..." - If you can, launch bots (you might not have try bot access, in that case ignore this line). - Mention a particular device in TEST= line. You can delete intermediate PatchSets (i.e. those with no comments), clicking on "delete" on the patchset frame uppermost right corner. (It facilitates review). https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:290: return false; indent. https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:315: // portions of the graph I think you should beef up this comment to add why you try to get the |capture_filter_| as MEDIATYPE_Interleaved first, and as MEDIATYPE_Video if that one fails. No need to mention "The following X calls". https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:325: DLOG_IF(ERROR, FAILED(hr)) << "Failed to find CapFilter:IAMStreamConfig: " Indenting -- l.322-326. 2 Spaces for new code block. Try to line up the operator<<. To avoid these circumstances you can use git cl format before upload [1]. [1] https://code.google.com/p/chromium/wiki/ClangFormat https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:330: video_stream_config_.Release(); ScopedComPtr will do this for you, i.e. at the end of this method, |video_stream_config_| goes out of scoped and is Release()d (like, delete). https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:448: Any particular reason to remove l.429-432 of the baseline? https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.h (right): https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.h:99: base::win::ScopedComPtr<ICaptureGraphBuilder2> capture_graph_builder; Member variables have a trailing underscore.
Patchset #2 (id:20001) has been deleted
Submitted Change Set 3 https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:290: return false; On 2015/03/30 19:12:17, mcasas wrote: > indent. Done. https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:315: // portions of the graph On 2015/03/30 19:12:17, mcasas wrote: > I think you should beef up this comment to add why you try > to get the |capture_filter_| as MEDIATYPE_Interleaved first, > and as MEDIATYPE_Video if that one fails. No need to mention > "The following X calls". Done. https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:325: DLOG_IF(ERROR, FAILED(hr)) << "Failed to find CapFilter:IAMStreamConfig: " On 2015/03/30 19:12:17, mcasas wrote: > Indenting -- l.322-326. 2 Spaces for new code block. Try to line up the > operator<<. > > To avoid these circumstances you can use git cl format before > upload [1]. > > [1] https://code.google.com/p/chromium/wiki/ClangFormat Done. https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:330: video_stream_config_.Release(); On 2015/03/30 19:12:16, mcasas wrote: > ScopedComPtr will do this for you, i.e. at the end of this > method, |video_stream_config_| goes out of scoped and > is Release()d (like, delete). Done. https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:448: On 2015/03/30 19:12:16, mcasas wrote: > Any particular reason to remove l.429-432 of the baseline? At l.416 HRESULT hr local variable is being checked. Since hr is not getting set again, this code is not necessary. https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.h (right): https://codereview.chromium.org/1039383002/diff/40001/media/video/capture/win... media/video/capture/win/video_capture_device_win.h:99: base::win::ScopedComPtr<ICaptureGraphBuilder2> capture_graph_builder; On 2015/03/30 19:12:17, mcasas wrote: > Member variables have a trailing underscore. Done.
The CQ bit was checked by ken.strickland@gmail.com
The CQ bit was unchecked by ken.strickland@gmail.com
LGTM with a few comments. Thanks! "TEST=Attach an Analog Video Capture devices," please rewrite. The bots go purple on you, meaning that you don't have try job permissions. I can request it for you if you plan to keep contributing. Since this CL affects current capture code, we'll have to keep an eye on the Canary-Dev-Beta channels as the CL rolls to Stable. Please try again Canary as soon as the CL rolls in (check https://omahaproxy.appspot.com/). https://codereview.chromium.org/1039383002/diff/60001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/60001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:91: ++i) { Since you touched this loop, please rewrite to C++11 (http://chromium-cpp.appspot.com/) for (const auto* property_name : kPropertyNames) { if (name.type() != VT_BSTR) prop_bag->Read(property_name, name.Receive(), 0); } https://codereview.chromium.org/1039383002/diff/60001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:311: // The following builds the upstream portions of the graph, "The following code"? https://codereview.chromium.org/1039383002/diff/60001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:316: // used first. "We try using the more prevalent MEDIATYPE_Interleaved first"?
Yes, I would like to contribute to the chromium code base on a regular basis. Please give try bot permissions. FYI, Using the USB2-Live device I still get a green bar at the bottom of the video stream only if I use one of the capabilities for 720x576. After extensive debugging all the chrome source looks good. https://codereview.chromium.org/1039383002/diff/60001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/60001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:91: ++i) { On 2015/03/31 18:11:56, mcasas wrote: > Since you touched this loop, please rewrite to C++11 > (http://chromium-cpp.appspot.com/) > > for (const auto* property_name : kPropertyNames) { > if (name.type() != VT_BSTR) > prop_bag->Read(property_name, name.Receive(), 0); > } Done. https://codereview.chromium.org/1039383002/diff/60001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:311: // The following builds the upstream portions of the graph, On 2015/03/31 18:11:56, mcasas wrote: > "The following code"? Done. https://codereview.chromium.org/1039383002/diff/60001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:316: // used first. On 2015/03/31 18:11:56, mcasas wrote: > "We try using the more prevalent MEDIATYPE_Interleaved first"? Done.
lgtm with the following addressed https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:179: device_names->push_back(Name(device_name, id)); where is the type direct_show now set when enumerating device names? https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:156: {kMediaSubTypeI420, PIXEL_FORMAT_I420}, did you run lint on this?
I have been running "git cl format", let me know if this is working out ok. Also with running the bots, is the command "git cl try" ok? Or does it take any kind of configuration on my side to specify which bots to run? https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:179: device_names->push_back(Name(device_name, id)); On 2015/04/01 06:41:30, perkj wrote: > where is the type direct_show now set when enumerating device names? Done. https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:156: {kMediaSubTypeI420, PIXEL_FORMAT_I420}, On 2015/04/01 06:41:30, perkj wrote: > did you run lint on this? I ran "git cl format" only. I'm not sure if that is hooked into cpplint.py or not. https://codereview.chromium.org/1039383002/diff/100001/media/video/capture/wi... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/100001/media/video/capture/wi... media/video/capture/win/video_capture_device_win.cc:319: base::win::ScopedComPtr<IAMStreamConfig> stream_config; I'm considering making this a class level variable because of it's later instantiation and use starting at l:462. I've made this change and tested it on my side with my limited test cases and it works fine. Should I mess with it?
"git cl try" runs a significant subset of all bots. git cl try --help will give you more info. You can't run bots yet because your try job permissions have not been approved. I suggest landing this CL since you got the LGTMs already. Again, since this changes code affecting working code, there might be unexpected regressions, so let's keep it focused. https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/80001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:156: {kMediaSubTypeI420, PIXEL_FORMAT_I420}, On 2015/04/01 12:46:04, ken.strickland wrote: > On 2015/04/01 06:41:30, perkj wrote: > > did you run lint on this? > > I ran "git cl format" only. I'm not sure if that is hooked into cpplint.py or > not. Cpplint is a syntax checker, clang-format changes the contents. I think cpplint is run on git cl upload. https://codereview.chromium.org/1039383002/diff/100001/media/video/capture/wi... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/1039383002/diff/100001/media/video/capture/wi... media/video/capture/win/video_capture_device_win.cc:319: base::win::ScopedComPtr<IAMStreamConfig> stream_config; On 2015/04/01 12:46:04, ken.strickland wrote: > I'm considering making this a class level variable because of it's later > instantiation and use starting at l:462. I've made this change and tested it on > my side with my limited test cases and it works fine. Should I mess with it? At any rate I'd recommend doing that in another CL. This one is LGTM'ed already.
The CQ bit was checked by ken.strickland@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org Link to the patchset: https://codereview.chromium.org/1039383002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1039383002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by ken.strickland@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1039383002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by ken.strickland@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1039383002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1039383002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ken.strickland@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1039383002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ken.strickland@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1039383002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1039383002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/773aa4541957d8ad7d28990005544e98ee534474 Cr-Commit-Position: refs/heads/master@{#323831} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
