Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2856893003:
Image Capture: wire getCapabilities() in Windows (Closed)
|
Created:
3 years, 7 months ago by mcasas Modified:
3 years, 7 months ago Reviewers:
robliao CC:
chromium-reviews, posciak+watch_chromium.org, chfremer+watch_chromium.org, xjz+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImage Capture: wire getCapabilities() in Windows
This CL implements the GetPhotoCapabilities() method for
Windows capture devices, by querying the appropriate
ICameraControl- and IVideoProcAmp-supporting nodes of the
|capture_filter_|.
Screenshots of the produced results vs the AmCap provided
values:
https://ibb.co/cfhCwQ
https://ibb.co/jNugqk
https://ibb.co/feiwO5
https://ibb.co/kShZAk
BUG=657128
Review-Url: https://codereview.chromium.org/2856893003
Cr-Commit-Position: refs/heads/master@{#469879}
Committed: https://chromium.googlesource.com/chromium/src/+/4461e2b935fde482f530465ffdce34c75aff6d50
Patch Set 1 #
Total comments: 12
Patch Set 2 : robliao@s first round of comments #
Total comments: 1
Patch Set 3 : robliao@ comments: splitted init'ing of ICameraControl IVideoProcAmp and added comment #Patch Set 4 : Rebase: ScopedComPtr s/QueryInterface/CopyTo/ #
Messages
Total messages: 27 (20 generated)
Description was changed from ========== Image Capture: wire getCapabilities() in Windows wip wip BUG= ========== to ========== Image Capture: wire getCapabilities() in Windows wip wip BUG=657128 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Image Capture: wire getCapabilities() in Windows wip wip BUG=657128 ========== to ========== Image Capture: wire getCapabilities() in Windows This CL implements the GetPhotoCapabilities() method for Windows capture devices, by querying the appropriate ICameraControl- and IVideoProcAmp-supporting nodes of the |capture_filter_|. Screenshots of the produced results vs the AmCap provided values: https://ibb.co/cfhCwQ https://ibb.co/jNugqk https://ibb.co/feiwO5 https://ibb.co/kShZAk BUG=657128 ==========
Patchset #1 (id:80001) has been deleted
mcasas@chromium.org changed reviewers: + robliao@chromium.org
robliao@ PTAL/Fwd appropriately, thanks!
https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:72: std::vector<mojom::MeteringMode>* supported_modes, You can use the default = nullptr argument to save on the nullptr, nullptrs below. https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:74: auto control_range = mojom::Range::New(); Will the caller be okay with any failure below? https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:78: if (!FAILED(hr)) { Use SUCCEEDED(hr) here and below. https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:518: hr = info->CreateNodeInstance(i, IID_PPV_ARGS(camera_control.Receive())); Is it possible for the node list to have two types KSNODETYPE_VIDEO_CAMERA_TERMINAL or KSNODETYPE_VIDEO_PROCESSING? https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:538: [camera_control](auto... args) { Optional: It might be better to just do the long*, long*, long* here or at least mention the number of expected arguments as both the getRange and get expect different sorts of args. maybe getRangeArgs and getArgs
ptal https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:72: std::vector<mojom::MeteringMode>* supported_modes, On 2017/05/03 21:38:26, robliao wrote: > You can use the default = nullptr argument to save on the nullptr, nullptrs > below. Done. https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:74: auto control_range = mojom::Range::New(); On 2017/05/03 21:38:27, robliao wrote: > Will the caller be okay with any failure below? Yes, returning an empty mojom::RangePtr is fine. https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:78: if (!FAILED(hr)) { On 2017/05/03 21:38:26, robliao wrote: > Use SUCCEEDED(hr) here and below. Done. https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:518: hr = info->CreateNodeInstance(i, IID_PPV_ARGS(camera_control.Receive())); On 2017/05/03 21:38:26, robliao wrote: > Is it possible for the node list to have two types > KSNODETYPE_VIDEO_CAMERA_TERMINAL or KSNODETYPE_VIDEO_PROCESSING? Yes, as a matter of fact it's the usual and expected situation, mirroring the USB underlying structure (https://tinyurl.com/uvc-1-5-specification), namely the "Camera Terminal" and "Processing Unit", respectively. It's so common to have both or none that if either of them fail to get mapped, it's not worth following and it's better to bail out as in l.521, 528 and 533. https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:538: [camera_control](auto... args) { On 2017/05/03 21:38:26, robliao wrote: > Optional: It might be better to just do the long*, long*, long* here or at least > mention the number of expected arguments as both the getRange and get expect > different sorts of args. > Correct, but if the calls in l.76 and l.89 would have the wrong number of parameters that'd be a compiler error. What I can do is to add inside this lambda e.g. a: static_assert(sizeof...(args) == 5, "Bla() needs 5 arguments"); (resp. with 2 arguments). WDYT? > maybe getRangeArgs and getArgs ?
https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:518: hr = info->CreateNodeInstance(i, IID_PPV_ARGS(camera_control.Receive())); On 2017/05/03 22:16:49, mcasas wrote: > On 2017/05/03 21:38:26, robliao wrote: > > Is it possible for the node list to have two types > > KSNODETYPE_VIDEO_CAMERA_TERMINAL or KSNODETYPE_VIDEO_PROCESSING? > > Yes, as a matter of fact it's the usual and expected > situation, mirroring the USB underlying structure > (https://tinyurl.com/uvc-1-5-specification), namely > the "Camera Terminal" and "Processing Unit", respectively. > > It's so common to have both or none that if either of > them fail to get mapped, it's not worth following and > it's better to bail out as in l.521, 528 and 533. In that case, you might want to clear camera_control and video_control in your loop then. If you hit this piece of code twice, you'll DCHECK/leak the pointer. https://codereview.chromium.org/2856893003/diff/100001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:538: [camera_control](auto... args) { On 2017/05/03 22:16:49, mcasas wrote: > On 2017/05/03 21:38:26, robliao wrote: > > Optional: It might be better to just do the long*, long*, long* here or at > least > > mention the number of expected arguments as both the getRange and get expect > > different sorts of args. > > > > Correct, but if the calls in l.76 and l.89 would > have the wrong number of parameters that'd be a > compiler error. What I can do is to add inside this > lambda e.g. a: > > static_assert(sizeof...(args) == 5, "Bla() needs 5 arguments"); > > (resp. with 2 arguments). > > WDYT? > > > maybe getRangeArgs and getArgs > > ? Well, it would error out anyways in a failure to satisfy the template. It's fine as is, but would be better documented if both lambdas didn't use "args".
lgtm + leak fix and nit. https://codereview.chromium.org/2856893003/diff/120001/media/capture/video/wi... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2856893003/diff/120001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:75: long min, max, step, default_value, flags, current; Nit: Define current with value_getter below.
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by mcasas@chromium.org
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2856893003/#ps180001 (title: "Rebase: ScopedComPtr s/QueryInterface/CopyTo/")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494022657920740,
"parent_rev": "41b169387a23719c61de5fba94c73f19ed9e30c7", "commit_rev":
"4461e2b935fde482f530465ffdce34c75aff6d50"}
Message was sent while issue was closed.
Description was changed from ========== Image Capture: wire getCapabilities() in Windows This CL implements the GetPhotoCapabilities() method for Windows capture devices, by querying the appropriate ICameraControl- and IVideoProcAmp-supporting nodes of the |capture_filter_|. Screenshots of the produced results vs the AmCap provided values: https://ibb.co/cfhCwQ https://ibb.co/jNugqk https://ibb.co/feiwO5 https://ibb.co/kShZAk BUG=657128 ========== to ========== Image Capture: wire getCapabilities() in Windows This CL implements the GetPhotoCapabilities() method for Windows capture devices, by querying the appropriate ICameraControl- and IVideoProcAmp-supporting nodes of the |capture_filter_|. Screenshots of the produced results vs the AmCap provided values: https://ibb.co/cfhCwQ https://ibb.co/jNugqk https://ibb.co/feiwO5 https://ibb.co/kShZAk BUG=657128 Review-Url: https://codereview.chromium.org/2856893003 Cr-Commit-Position: refs/heads/master@{#469879} Committed: https://chromium.googlesource.com/chromium/src/+/4461e2b935fde482f530465ffdce... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://chromium.googlesource.com/chromium/src/+/4461e2b935fde482f530465ffdce... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
