Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479413002:
Image Capture v4l2: reset all user controls to default values when closing device fd (Closed)
|
Created:
4 years, 1 month ago by mcasas Modified:
4 years ago Reviewers:
emircan CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, xjz+watch_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImage Capture v4l2: reset all user controls to default values when closing device fd
This CL adds logic for resetting all User Controls to its default
value upon closing the device file descriptor; otherwise, as the
bug proves, they persist across camera uses, e.g. the zoom level
is unchanged.
This would be relatively simple except for the fact that controls
come in two types: user controls and camera controls, and
that some controls need another one to be set or cleared
beforehand (e.g. white_balance_auto must be set to off
before white_balance can be set, etc).
See [1] for Spec
[1] https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single/v4l2.html#control
BUG=662616
Committed: https://crrev.com/9b3a179c91492c28c2eaf53daf52d6caebff77a0
Cr-Commit-Position: refs/heads/master@{#437695}
Patch Set 1 #
Total comments: 12
Patch Set 2 : emircan@ comments and using while-loops ISO for-loops #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 31 (23 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Image Capture v4l2: reset all user controls to default values when closing device fd This CL adds logic for resetting all User Controls to its default value upon closing the device file descriptor; otherwise, as the bug proves, they persist across camera uses, e.g. the zoom level is unchanged. See [1] for Spec [1] https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single... BUG=662616 ========== to ========== Image Capture v4l2: reset all user controls to default values when closing device fd This CL adds logic for resetting all User Controls to its default value upon closing the device file descriptor; otherwise, as the bug proves, they persist across camera uses, e.g. the zoom level is unchanged. This would be relatively simple except for the fact that controls come in two types: user controls and camera controls, and that some controls need another one to be set or cleared beforehand (e.g. white_balance_auto must be set to off before white_balance can be set, etc). See [1] for Spec [1] https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single... BUG=662616 ==========
Patchset #1 (id:80001) has been deleted
mcasas@chromium.org changed reviewers: + emircan@chromium.org
emircan@ can you PTAL?
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...
Looking good, minor comments. https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:56: const int kMaxIOCtrlRetries = 5; Add a comment for this here as well. https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:142: if (control_id == V4L2_CID_AUTO_WHITE_BALANCE) Can you regroup this into a switch statement? https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:165: HANDLE_EINTR(ioctl(device_fd, VIDIOC_S_CTRL, &auto_white_balance)) < 0; Would it help to log the return value here? https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:177: if (IsSpecialControl(control_id)) Can you refactor l.177-187 and l.226-236 into a static method? You can even use it in the unittest. https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:225: control_id < V4L2_CID_CAMERA_CLASS_BASE + 32; ++control_id) { Where is this 32 coming from? I was doing a quick search on this and it looks like it can extend to 33: http://lxr.free-electrons.com/source/include/uapi/linux/v4l2-controls.h#L760 https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... File media/capture/video/linux/v4l2_capture_delegate_unittest.cc (right): https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate_unittest.cc:29: if (control_id == V4L2_CID_AUTO_WHITE_BALANCE) switch statement?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #2 (id:120001) has been deleted
emircan@ ptal (never mind the bots if red: waterfall closed) https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:56: const int kMaxIOCtrlRetries = 5; On 2016/12/07 21:45:31, emircan wrote: > Add a comment for this here as well. Done. https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:142: if (control_id == V4L2_CID_AUTO_WHITE_BALANCE) On 2016/12/07 21:45:31, emircan wrote: > Can you regroup this into a switch statement? Done. https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:165: HANDLE_EINTR(ioctl(device_fd, VIDIOC_S_CTRL, &auto_white_balance)) < 0; On 2016/12/07 21:45:31, emircan wrote: > Would it help to log the return value here? Not really, DPLOG() in l.167 will show what has happened better than us. https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:177: if (IsSpecialControl(control_id)) On 2016/12/07 21:45:31, emircan wrote: > Can you refactor l.177-187 and l.226-236 into a static method? You can even use > it in the unittest. I've rewritten the whole implementation so it's hopefully simpler, and it's based on the --according to the online doc-- standard way of iterating over the controls using a while and some flag-bit logic. https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:225: control_id < V4L2_CID_CAMERA_CLASS_BASE + 32; ++control_id) { On 2016/12/07 21:45:31, emircan wrote: > Where is this 32 coming from? I was doing a quick search on this and it looks > like it can extend to 33: > http://lxr.free-electrons.com/source/include/uapi/linux/v4l2-controls.h#L760 In the latest kernel yes, but in the 3.13 that we use for compiling, there's just 32 [1]. However, thanks for pointing this out, the way to work with the Camera Controls is to iterate over them, not to try a fixed number, so, changed. [1] http://lxr.free-electrons.com/source/include/uapi/linux/v4l2-controls.h?v=3.13 https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... File media/capture/video/linux/v4l2_capture_delegate_unittest.cc (right): https://codereview.chromium.org/2479413002/diff/100001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate_unittest.cc:29: if (control_id == V4L2_CID_AUTO_WHITE_BALANCE) On 2016/12/07 21:45:31, emircan wrote: > switch statement? Done.
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...
lgtm % nit https://codereview.chromium.org/2479413002/diff/140001/media/capture/video/li... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2479413002/diff/140001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:212: while (0 == HANDLE_EINTR(ioctl(device_fd, VIDIOC_QUERYCTRL, &range))) { This is different than before, as loop will exit when there is an error. Earlier it was a continue statement.
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...
https://codereview.chromium.org/2479413002/diff/140001/media/capture/video/li... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2479413002/diff/140001/media/capture/video/li... media/capture/video/linux/v4l2_capture_delegate.cc:212: while (0 == HANDLE_EINTR(ioctl(device_fd, VIDIOC_QUERYCTRL, &range))) { On 2016/12/09 19:19:37, emircan wrote: > This is different than before, as loop will exit when there is an error. Earlier > it was a continue statement. Before we skipped the failing controls because we were touching all of them and some might be not implemented, but now, due to the nature of V4L2_CTRL_FLAG_NEXT_CTRL, we should not fail at all.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org Link to the patchset: https://codereview.chromium.org/2479413002/#ps160001 (title: " ")
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": 160001, "attempt_start_ts": 1481324641793760,
"parent_rev": "57a12d5fcce2ba3a0075e609a22c56a056e5d93d", "commit_rev":
"77b974dc493f72ccaa32b0385d89666fafa21e61"}
Message was sent while issue was closed.
Description was changed from ========== Image Capture v4l2: reset all user controls to default values when closing device fd This CL adds logic for resetting all User Controls to its default value upon closing the device file descriptor; otherwise, as the bug proves, they persist across camera uses, e.g. the zoom level is unchanged. This would be relatively simple except for the fact that controls come in two types: user controls and camera controls, and that some controls need another one to be set or cleared beforehand (e.g. white_balance_auto must be set to off before white_balance can be set, etc). See [1] for Spec [1] https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single... BUG=662616 ========== to ========== Image Capture v4l2: reset all user controls to default values when closing device fd This CL adds logic for resetting all User Controls to its default value upon closing the device file descriptor; otherwise, as the bug proves, they persist across camera uses, e.g. the zoom level is unchanged. This would be relatively simple except for the fact that controls come in two types: user controls and camera controls, and that some controls need another one to be set or cleared beforehand (e.g. white_balance_auto must be set to off before white_balance can be set, etc). See [1] for Spec [1] https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single... BUG=662616 Review-Url: https://codereview.chromium.org/2479413002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Image Capture v4l2: reset all user controls to default values when closing device fd This CL adds logic for resetting all User Controls to its default value upon closing the device file descriptor; otherwise, as the bug proves, they persist across camera uses, e.g. the zoom level is unchanged. This would be relatively simple except for the fact that controls come in two types: user controls and camera controls, and that some controls need another one to be set or cleared beforehand (e.g. white_balance_auto must be set to off before white_balance can be set, etc). See [1] for Spec [1] https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single... BUG=662616 Review-Url: https://codereview.chromium.org/2479413002 ========== to ========== Image Capture v4l2: reset all user controls to default values when closing device fd This CL adds logic for resetting all User Controls to its default value upon closing the device file descriptor; otherwise, as the bug proves, they persist across camera uses, e.g. the zoom level is unchanged. This would be relatively simple except for the fact that controls come in two types: user controls and camera controls, and that some controls need another one to be set or cleared beforehand (e.g. white_balance_auto must be set to off before white_balance can be set, etc). See [1] for Spec [1] https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single... BUG=662616 Committed: https://crrev.com/9b3a179c91492c28c2eaf53daf52d6caebff77a0 Cr-Commit-Position: refs/heads/master@{#437695} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9b3a179c91492c28c2eaf53daf52d6caebff77a0 Cr-Commit-Position: refs/heads/master@{#437695} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
