V4L2: avoid resetting zoom controls

V4L2: avoid resetting zoom controls

When resetting the camera controls, the zoom setting should be skipped
just like the pan and tilt controls.

This is a continuation of crbug/697885 where the pan and tilt control became
skipped when the camera is reset.

BUG=716122
Review-Url: https://codereview.chromium.org/2904503002
Cr-Commit-Position: refs/heads/master@{#475470}
Committed: https://chromium.googlesource.com/chromium/src/+/9878f1e3cc8de255d69c89af711a010aad67edf7

[tovep, 2017-05-23 13:54:40]

The CQ bit was checked by tovep@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 13:54:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 15:12:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 15:12:49]

Dry run: This issue passed the CQ dry run.

[tovep, 2017-05-23 18:06:36]

tovep@chromium.org changed reviewers:
+ mcasas@chromium.org, posciak@chromium.org

[tovep, 2017-05-23 18:06:37]

Hi Miguel, Pawel

Miguel: Could you please review the addition of the zoom controls to the
blacklist of pan/tilt controls that you added?

Pawel: Feel free to take a look at the change to the extent that you like.  I
assume I need an approval from you too as the owner of the directory.

Thanks

Tove

[Pawel Osciak, 2017-05-25 05:42:43]

https://codereview.chromium.org/2904503002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/v4l2_capture_delegate.cc (right):

https://codereview.chromium.org/2904503002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/v4l2_capture_delegate.cc:187: static bool
IsBlacklistedControl(int control_id) {
Would it perhaps be possible to have this list in a common place to avoid
duplicating it here and in the test?

Or, perhaps better, call the same method in both?

[tovep, 2017-05-26 08:39:47]

We could declare IsBlacklistedControl() in v4l2_capture_delegate.h and call it
from IsSpecialOrBlacklistedControl() in v4l2_capture_delegate_unittest.cc (the
latter includes more controls).

However, I feel a bit hesitant to make this function part of the public api. The
kernel patches applied in
crrev.com/c/461841/ renders this fix unnecessary for devices with the 3.14
kernel, and that patch could possibly be applied also to the other kernels we
need for the CfMs, e.g., 3.8. I would like to investigate the interactions
between this fix and the kernel changes more deeply. Perhaps the black list
could be removed altogether, including the pan/tilt controls, if the PTZ
controls works fine without it on a patched kernel.

So I suggest that we keep it duplicated for now, I'll investigate the
interaction with the kernel patch more deeply, and if we then decide that the
blacklist should remain, I'll make a new CL to expose IsBlacklistedControl() in
the header-file.

On 2017/05/25 05:42:43, Pawel Osciak wrote:
>
https://codereview.chromium.org/2904503002/diff/1/media/capture/video/linux/v...
> File media/capture/video/linux/v4l2_capture_delegate.cc (right):
> 
>
https://codereview.chromium.org/2904503002/diff/1/media/capture/video/linux/v...
> media/capture/video/linux/v4l2_capture_delegate.cc:187: static bool
> IsBlacklistedControl(int control_id) {
> Would it perhaps be possible to have this list in a common place to avoid
> duplicating it here and in the test?
> 
> Or, perhaps better, call the same method in both?

[mcasas, 2017-05-26 14:25:51]

https://codereview.chromium.org/2904503002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/v4l2_capture_delegate.cc (right):

https://codereview.chromium.org/2904503002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/v4l2_capture_delegate.cc:187: static bool
IsBlacklistedControl(int control_id) {
On 2017/05/25 05:42:43, Pawel Osciak wrote:
> Would it perhaps be possible to have this list in a common place to avoid
> duplicating it here and in the test?
> 
> Or, perhaps better, call the same method in both?

I ruled that out because, for the purpose of 
unittests, we're not supposed to use the class
under test, but instead see it as a black box.
IOW, we can't test
FunctionA() {
  return FunctionB();
}

by comparing the result of calling FunctionA()
with the result of calling FunctionB(), that's not
testing anything :-)
Instead, we test the result of calling FunctionA()
against some expectations, that are only defined
for the purpose of the test.

[Pawel Osciak, 2017-05-29 03:54:44]

lgtm

https://codereview.chromium.org/2904503002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/v4l2_capture_delegate.cc (right):

https://codereview.chromium.org/2904503002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/v4l2_capture_delegate.cc:187: static bool
IsBlacklistedControl(int control_id) {
On 2017/05/26 14:25:51, mcasas wrote:
> On 2017/05/25 05:42:43, Pawel Osciak wrote:
> > Would it perhaps be possible to have this list in a common place to avoid
> > duplicating it here and in the test?
> > 
> > Or, perhaps better, call the same method in both?
> 
> I ruled that out because, for the purpose of 
> unittests, we're not supposed to use the class
> under test, but instead see it as a black box.
> IOW, we can't test
> FunctionA() {
>   return FunctionB();
> }
> 
> by comparing the result of calling FunctionA()
> with the result of calling FunctionB(), that's not
> testing anything :-)
> Instead, we test the result of calling FunctionA()
> against some expectations, that are only defined
> for the purpose of the test.

Acknowledged.

[tovep, 2017-05-29 07:59:46]

Pawel, thank you for the review.

Miguel, do you have any additional comments regarding the change, or does
it look fine to you?

Thanks

Tove

On Mon, May 29, 2017 at 5:54 AM, <posciak@chromium.org> wrote:

> lgtm
>
>
> https://codereview.chromium.org/2904503002/diff/1/media/
> capture/video/linux/v4l2_capture_delegate.cc
> File media/capture/video/linux/v4l2_capture_delegate.cc (right):
>
> https://codereview.chromium.org/2904503002/diff/1/media/
> capture/video/linux/v4l2_capture_delegate.cc#newcode187
> media/capture/video/linux/v4l2_capture_delegate.cc:187: static bool
> IsBlacklistedControl(int control_id) {
> On 2017/05/26 14:25:51, mcasas wrote:
> > On 2017/05/25 05:42:43, Pawel Osciak wrote:
> > > Would it perhaps be possible to have this list in a common place to
> avoid
> > > duplicating it here and in the test?
> > >
> > > Or, perhaps better, call the same method in both?
> >
> > I ruled that out because, for the purpose of
> > unittests, we're not supposed to use the class
> > under test, but instead see it as a black box.
> > IOW, we can't test
> > FunctionA() {
> > return FunctionB();
> > }
> >
> > by comparing the result of calling FunctionA()
> > with the result of calling FunctionB(), that's not
> > testing anything :-)
> > Instead, we test the result of calling FunctionA()
> > against some expectations, that are only defined
> > for the purpose of the test.
>
> Acknowledged.
>
> https://codereview.chromium.org/2904503002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mcasas, 2017-05-29 16:25:09]

lgtm

[tovep, 2017-05-30 07:00:27]

The CQ bit was checked by tovep@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 07:00:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 08:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 08:30:51]

Dry run: This issue passed the CQ dry run.

[tovep, 2017-05-30 08:47:47]

The CQ bit was checked by tovep@chromium.org

[commit-bot: I haz the power, 2017-05-30 08:48:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 08:51:05]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1496134067589600, "parent_rev":
"febb8e88d2e3617cede29d793ca171f7a751bb88", "commit_rev":
"9878f1e3cc8de255d69c89af711a010aad67edf7"}

[commit-bot: I haz the power, 2017-05-30 08:51:13]

Description was changed from

==========
V4L2: avoid resetting zoom controls

When resetting the camera controls, the zoom setting should be skipped
just like the pan and tilt controls.

This is a continuation of crbug/697885 where the pan and tilt control became
skipped when the camera is reset.

BUG=716122
==========

to

==========
V4L2: avoid resetting zoom controls

When resetting the camera controls, the zoom setting should be skipped
just like the pan and tilt controls.

This is a continuation of crbug/697885 where the pan and tilt control became
skipped when the camera is reset.

BUG=716122

Review-Url: https://codereview.chromium.org/2904503002
Cr-Commit-Position: refs/heads/master@{#475470}
Committed:
https://chromium.googlesource.com/chromium/src/+/9878f1e3cc8de255d69c89af711a...
==========

[commit-bot: I haz the power, 2017-05-30 08:51:15]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/9878f1e3cc8de255d69c89af711a...