New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 893494 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 8
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature

Blocking:
issue 852302
issue 893501



Sign in to add a comment

capture_unittests should fail if no camera is found

Project Member Reported by keiichiw@chromium.org, Oct 9

Issue description

Capture_unittests currently returns when no camera is found in test cases which require camera devices (e.g. [1]). So, capture_unittests cannot catch bugs in the feature for enumerating devices.

However, just changing them into failures won't work because capture_unittests is running even on devices that have no camera indeed (e.g. Chromebox).

To resolve this problem, we should split capture_unittests to 2 suites; one that doesn't require a camera and one that does.
The latter suite will run only on devices that has cameras.

[1] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device_unittest.cc?l=505&rcl=e89480287d6a22f042d96c54586004e6ef861a2a
 
Components: OS>Kernel>Camera Internals>Media>Capture
Blocking: 893501
chfremer@, I assign this issue to you for now. When we decide how we will work on this, let's re-assign.

I'm not sure how we should split the current_unittests.
Should they be different binaries? Or, should we use command line parameters to specify which tests run? Any idea?
I think a command-line parameter or a test case name prefix would be suitable.
We already started using the prefix UsingRealWebcam_ in several places to distinguish tests that try to access real capture devices in order to make sure we run them sequentially and never in parallel.

Maybe a prefix RequiresRealCamera_ could be used here.

Another question: Is the only reason we want to do this split to be able to get coverage of code relevant for enumerating devices? If that is the case, would we be able to get such coverage without requiring real cameras, e.g. by mocking out a user-space API as done in https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_device_factory_linux_unittest.cc?dr=CSs&g=0&l=91 ?
Using a prefix sounds reasonable. Then, we can filter them by gtest's command-line option.
But, I wonder if we should distinguish "UsingRealWebcam_" and "RequiresRealCamera_". 
Is there any special reason for this? "RequiresRealCamera_" tests also need to be run sequentially, I think.

--
Another reason for splitting tests is that I don't think tests should say "PASS" when they are not actually performed.
For example, a test "TakePhoto" passes when no camera is found *although* any photo feature is not exercised. It's confusing.

Also, we can save time and resources in test infra by splitting test suites, because we can avoid running "UsingRealWebcam" tests on devices that have no camera.
Just to build on top of what Keiichi said, we generally want to avoid cases where a part of the underlying software stack has a bug which leads the camera not to be detected, but the tests keep passing, because the test setup just invisibly skips them if no camera is present.
I agree, we don't need two different prefixes, as long as we can make sure to correctly configure the gtest_filter command-line option everywhere where such tests are run. And I also agree it is problematic to have tests quietly skipping.

I have to admit that I don't think I know all those places, though. The Chromium CQ is certainly one important place, and the other is the webrtc builders, i.e. buildbot/chromium.webrtc.fyi/* and buildbot/chromium.webrtc/* on https://ci.chromium.org/p/chromium/builders. But there may be others that I am not aware of.
Cc: phoglund@chromium.org oprypin@chromium.org
+oprypin@ and phoglund@, who are owners of buildbot/chromium.webrtc/.
Note that this discussion came from [1] in go/cros-camera-waterfall-test.

---

I took a look at /src/testing/buildbot/test_suites.pyl and found that capture_unittests are used in three tests:
- chromeos_gtests [2]
- chromium_gtests [3]
- webrtc_chromium_baremetal_gtests [4]

"chromeos_gtests" is the one that I'm working on.
Since I'm planning to use vivid (virtual video input device) for it, we will be able to run "UsingRealWebcam_" tests there.

In "chromium_gtests", capture_unittests is called without "--test-launcher-jobs=1", which is required when a real webcam is used IIUC.
So, I guess chromium_gtests isn't used for platforms that have real cameras. Then, we should exclude "UsingRealWebcam_" tests here.

For "webrtc_chromium_baremetal_gtests", we can run "UsingRealWebcam_" tests because the comment says that their bots have real webcams.

According to the search result [5], waterfalls.pyl also includes some "capture_unittests". However, it seems that they just build and don't run capture_unittests because they are in "additional_compile_targets".

---

So, I think we can achieve this task by the following way:
* Add the prefix "UsingRealWebcam_" to test cases in capture_unittests that use real webcams
* Make "UsingRealWebcam_" tests fail when no camera is found
* Add a command-line option to exclude "UsingRealWebcam_" tests for "chromium_gtests".

Does it make sense?

[1] https://docs.google.com/a/google.com/document/d/1h8BEmDdUzfbaIR6_4WiBBh_gNOb8gVdJi1kX8SbdoTk/edit?disco=AAAACPba4BA
[2] https://cs.chromium.org/chromium/src/testing/buildbot/test_suites.pyl?l=368&rcl=f34485ffdec1a7ac058941794747fbec1c83ece6
[3] https://cs.chromium.org/chromium/src/testing/buildbot/test_suites.pyl?l=567&rcl=f34485ffdec1a7ac058941794747fbec1c83ece6
[4] https://cs.chromium.org/chromium/src/testing/buildbot/test_suites.pyl?l=2280&rcl=f34485ffdec1a7ac058941794747fbec1c83ece6
[5] https://cs.chromium.org/search/?q=capture_unittest+file:pyl+file:%5Esrc/testing/buildbot/+package:%5Echromium$&type=cs

Cc: bpastene@chromium.org chfremer@chromium.org
Owner: keiichiw@chromium.org
Status: Started (was: Untriaged)
I create a CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1276407
Note to self: After this change, we will need to exclude CaptureMjpeg when we run capture_unittests on ChromeOS VM with vivid.
re #8: Thanks for the thorough analysis. Yes I think this makes sense.
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dfeab488cdc95233b19b9edeedef3d0b7581b17e

commit dfeab488cdc95233b19b9edeedef3d0b7581b17e
Author: Keiichi Watanabe <keiichiw@chromium.org>
Date: Fri Oct 12 04:50:26 2018

[Video Capture, Test] Make tests requiring real webcam fail if it's not found

Use prefix "UsingRealWebcam_" for names of tests that require real webcam and
add checking whether webcam is found or not.
Accordingly, exclude such test cases from test suites for devices that have no
camera.

Bug:  893494 
Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebox (guado)
      and it failed.
Test: Ran capture_unittests excluding UsingRealWebcam_ tests on Chromebox (guado)
      and it passed.
Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebook (kevin)
      and it passed.

Change-Id: I5c91380b7674aeace6a7d1764f7fad31f02f8af7
Reviewed-on: https://chromium-review.googlesource.com/c/1276407
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599097}
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/media/capture/video/video_capture_device_unittest.cc
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.android.fyi.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.android.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.chromiumos.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.mac.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.win.json
[modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/test_suites.pyl

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bb43df173039894cf46c8b3476913f0f20ae6d1

commit 7bb43df173039894cf46c8b3476913f0f20ae6d1
Author: Henrik Grunell <grunell@chromium.org>
Date: Fri Oct 12 06:57:19 2018

Revert "[Video Capture, Test] Make tests requiring real webcam fail if it's not found"

This reverts commit dfeab488cdc95233b19b9edeedef3d0b7581b17e.

Reason for revert: Tests fail on Chromium WebRTC bots. Example: https://ci.chromium.org/buildbot/chromium.webrtc/Mac%20Tester/84091

Original change's description:
> [Video Capture, Test] Make tests requiring real webcam fail if it's not found
> 
> Use prefix "UsingRealWebcam_" for names of tests that require real webcam and
> add checking whether webcam is found or not.
> Accordingly, exclude such test cases from test suites for devices that have no
> camera.
> 
> Bug:  893494 
> Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebox (guado)
>       and it failed.
> Test: Ran capture_unittests excluding UsingRealWebcam_ tests on Chromebox (guado)
>       and it passed.
> Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebook (kevin)
>       and it passed.
> 
> Change-Id: I5c91380b7674aeace6a7d1764f7fad31f02f8af7
> Reviewed-on: https://chromium-review.googlesource.com/c/1276407
> Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
> Reviewed-by: Christian Fremerey <chfremer@chromium.org>
> Reviewed-by: Ben Pastene <bpastene@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#599097}

TBR=bpastene@chromium.org,chfremer@chromium.org,keiichiw@chromium.org

Change-Id: I282dc00ffad2ff6614e7dfab1a9404da85cf032a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  893494 
Reviewed-on: https://chromium-review.googlesource.com/c/1278272
Reviewed-by: Henrik Grunell <grunell@chromium.org>
Commit-Queue: Henrik Grunell <grunell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599123}
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/media/capture/video/video_capture_device_unittest.cc
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.android.fyi.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.android.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.chromiumos.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.mac.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.win.json
[modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/test_suites.pyl

The CL in #12 caused failures on Win/Mac builders in chromium.webrtc and was reverted by #13.
There is a discussion on https://chromium-review.googlesource.com/c/chromium/src/+/1278272/1/testing/buildbot/test_suites.pyl#2282.

I submitted the fixed version of CL: crrev.com/c/1278432
The webrtc bots seem to have bots called "... Builder", e.g. https://ci.chromium.org/buildbot/chromium.webrtc/Linux%20Builder, and bots called "... Tester", e.g. https://ci.chromium.org/buildbot/chromium.webrtc/Linux%20Tester. I assume that the ones called Builder do not have webcams, but the ones called Tester do. We need to make sure that the tests keep running on the ones called Tester. Otherwise we lose crucial test coverage.
#15: 
IIUC, while Builders just do build and don't run tests, Testers do actually tests.
For example, this is my reverted CL's log on Mac Builder:
https://ci.chromium.org/buildbot/chromium.webrtc/Mac%20Builder/104344
As far as I see, it only built files and capture_unittests didn't run here.

You can see Builders as "bld" and Testers as "tst" here.
https://ci.chromium.org/p/chromium/g/chromium.webrtc/console

So, I think we only need to consider Testers for this issue.
In my new CL (crrev.com/c/1278612), I added a filter in "AddTestSpec", which is used for Testers' settings.
I found a document about Buiders and Testers, which seems to help my comment #16:
https://www.chromium.org/developers/testing/chromium-build-infrastructure/tour-of-the-chromium-buildbot#TOC-Builders-vs-Testers
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/ac1ce788ac4d015fa6ce23d3438337ab12ac07ce

commit ac1ce788ac4d015fa6ce23d3438337ab12ac07ce
Author: Keiichi Watanabe <keiichiw@chromium.org>
Date: Mon Oct 15 07:09:16 2018

Stop running capture unittests requiring real webcams in chromium.webrtc

We decided to make capture_unittests's tests fail if no webcam is found.
Since real webcams seem unavailable on Windows/Mac builders (cf. CL:1278272),
we exclude such test cases there in chromium.webrtc.

Bug:  893494 
Change-Id: I98a6e107a230726fdd9dd54c89e4a0048bdfd14d
Reviewed-on: https://chromium-review.googlesource.com/c/1278612
Reviewed-by: Oleh Prypin <oprypin@chromium.org>
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Auto-Submit: Keiichi Watanabe <keiichiw@chromium.org>

[modify] https://crrev.com/ac1ce788ac4d015fa6ce23d3438337ab12ac07ce/scripts/slave/recipe_modules/chromium_tests/chromium_webrtc.py

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b332e81bd632b21d2cbb4634f30e08768b687bb5

commit b332e81bd632b21d2cbb4634f30e08768b687bb5
Author: Keiichi Watanabe <keiichiw@chromium.org>
Date: Mon Oct 15 09:25:34 2018

[Video Capture, Test] Make tests requiring real webcam fail if it's not found

Use prefix "UsingRealWebcam_" for names of tests that require real webcam and
add checking whether webcam is found or not.
Accordingly, exclude such test cases from test suites for devices that have no
camera.

      and it failed.
      and it passed.
      and it passed.

Bug:  893494 
Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebox (guado)
Test: Run capture_unittests excluding UsingRealWebcam_ tests on Chromebox (guado)
Test: Run capture_unittests including UsingRealWebcam_ tests on Chromebook (kevin)
Change-Id: I9ba9657b47afadf6f3128cdcb91e74e1793040b4
Reviewed-on: https://chromium-review.googlesource.com/c/1278432
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599596}
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/media/capture/video/video_capture_device_unittest.cc
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.android.fyi.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.android.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.chromiumos.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.linux.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.mac.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.win.json
[modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/test_suites.pyl

Status: Fixed (was: Started)
The updated capture_unittests passed also on Win/Mac testers.
Win 10: https://ci.chromium.org/buildbot/chromium.webrtc/Win10%20Tester/32937
Mac:    https://ci.chromium.org/buildbot/chromium.webrtc/Mac%20Tester/84196

So, I'll mark this issue fixed.
Status: Assigned (was: Fixed)
Looks like the tests are flaky on Windows:

../../media/capture/video/video_capture_device_unittest.cc(642): error: Value of: device_descriptor

Most of the tests in https://logs.chromium.org/logs/chromium/bb/chromium.webrtc/Win8_Tester/45883/+/recipes/steps/capture_unittests/0/logs/VideoCaptureDeviceTests_VideoCaptureDeviceTest.UsingRealWebcam_CaptureMjpeg_1/0 pass but not all of them. See https://ci.chromium.org/buildbot/chromium.webrtc/Win8%20Tester/45883.
Hmm. After Build #45883, capture_unittests hasn't failed until the current latest build(#45950) on Win8 Tester.
https://ci.chromium.org/buildbot/chromium.webrtc/Win8%20Tester/?limit=200

Though we might want to keep watching it for more a few days, I'm not sure we should call it "flaky".

> error: Value of: device_descriptor
This message means that no real camera is available. I think there is no way to avoid this kind of problems completely as long as we use real devices.
So, perhaps, I guess we should not care this kind of errors if it doesn't happen frequently.
How do you think?
Yeah, I'm not sure if they're actually flaky, rather they're just broken.

Using real devices has been working fine on other platforms and our webcam browsertest appears to work: https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_webcam_browsertest.cc?sq=package:chromium&g=0

On the other hand, they just skip rather than fail if there is no real device, so it's possible we just haven't seen the errors.

I'll disable the capture_unittests tests on Win for now and enable for the other platforms on the WebRTC bots.
Project Member

Comment 24 by bugdroid1@chromium.org, Nov 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f

commit f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f
Author: Patrik Höglund <phoglund@chromium.org>
Date: Wed Nov 07 14:17:50 2018

Re-enable capture_unittests on WebRTC bots except windows.

The tests are clearly broken on Windows if a webcam is plugged in.
They pass on win10 but only because they're disabled there. See bug
for example breakages on Windows.

Bug:  893494 
Change-Id: I8ea43cc4e3ef23082246ff23ec635f2ec5bfbbe2
Reviewed-on: https://chromium-review.googlesource.com/c/1309789
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606038}
[modify] https://crrev.com/f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f/media/capture/video/video_capture_device_unittest.cc
[modify] https://crrev.com/f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f/testing/buildbot/chromium.webrtc.json
[modify] https://crrev.com/f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f/testing/buildbot/test_suites.pyl

Components: Infra>Client>WebRTC
Owner: phoglund@chromium.org
Can we close this issue? Or, do you want to keep it open until we re-enable it on windows? If it's the latter case, we perhaps might want to change the summary of this issue.

Anyway, let me change the owner to phoglund@.
Status: Fixed (was: Assigned)
I don't plan to work more on this. I think we can close it even though there's a TODO pointing to it. Perhaps some kind soul in the future will take up fixing the tests on win...
Project Member

Comment 27 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c02508024cd68c9c350777a4d0d70af9f0ce731d

commit c02508024cd68c9c350777a4d0d70af9f0ce731d
Author: Patrik Höglund <phoglund@chromium.org>
Date: Thu Nov 08 12:23:04 2018

Correct disable statement for CaptureWithSize.

I missed 4 (!) cases when disabling this test for win, so let's fix
that.

Bug:  893494 
Change-Id: I6ebfe7d2db4462b4da378aa6a39497b60025f060
Tbr: emircan@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1326146
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606420}
[modify] https://crrev.com/c02508024cd68c9c350777a4d0d70af9f0ce731d/media/capture/video/video_capture_device_unittest.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35b85773cd9e66481ad68b8210118435cf7891a8

commit 35b85773cd9e66481ad68b8210118435cf7891a8
Author: Patrik Höglund <phoglund@chromium.org>
Date: Thu Nov 08 14:31:21 2018

Make CaptureWithSize actually be disabled for real this time.

Bug:  893494 
Change-Id: Ia1d28a678058ce949726e71f511f536482c2eee9
Tbr: emircan@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1326461
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606458}
[modify] https://crrev.com/35b85773cd9e66481ad68b8210118435cf7891a8/media/capture/video/video_capture_device_unittest.cc

Sign in to add a comment