New issue
Advanced search Search tips

Issue 899595 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Nov 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 904730

Blocking:
issue 899586



Sign in to add a comment

Migrate video_VideoCapability to Tast

Project Member Reported by hiroh@chromium.org, Oct 29

Issue description

video_VideoCapability checks the capability of autotest-capability and labels detected by avtest_label_detect.
This test ensures that static capability settings are correct.
 
Blocking: 899586
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/859c074de80e5eb071627f7e8c30dd42e4e955f9

commit 859c074de80e5eb071627f7e8c30dd42e4e955f9
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed Oct 31 19:42:40 2018

autocaps: Add DefaultCapabilityDir variable

DefaultCapabilityDir is the directory where yaml files are installed by
autotest-capability.

BUG= chromium:899595 
TEST=video.Capability

Change-Id: I4593bf6fe6989f5333aade52d7c4ca8a51cf18bd
Reviewed-on: https://chromium-review.googlesource.com/1307193
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/859c074de80e5eb071627f7e8c30dd42e4e955f9/src/chromiumos/cmd/local_test_runner/main.go
[modify] https://crrev.com/859c074de80e5eb071627f7e8c30dd42e4e955f9/src/chromiumos/tast/autocaps/caps.go

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/63afcc122153ced974bcfe4234a49fd4a48bce36

commit 63afcc122153ced974bcfe4234a49fd4a48bce36
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu Nov 01 04:59:25 2018

video.Capability: Add video_VideoCapability in Tast

video.Capability is equivalent to video_VideoCapability in autotest.
This ensures the capabilities computed by autocaps package are correct.

BUG= chromium:899595 
TEST=video.Capability on kevin
CQ-DEPEND=CL:1307193, CL:1297560

Change-Id: I2259b810533f86df682234db86517715c9486025
Reviewed-on: https://chromium-review.googlesource.com/1303780
Commit-Ready: Hirokazu Honda <hiroh@chromium.org>
Tested-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/63afcc122153ced974bcfe4234a49fd4a48bce36/src/chromiumos/tast/local/bundles/cros/video/capability.go
[modify] https://crrev.com/63afcc122153ced974bcfe4234a49fd4a48bce36/src/chromiumos/tast/local/bundles/cros/video/lib/caps/caps.go

video.Capability is flaky on kepler devices.
I think it doesn't add capabilities on kepler case properly.
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/233aa756261306f0d22d37b29ec94d63366a0abb

commit 233aa756261306f0d22d37b29ec94d63366a0abb
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed Nov 07 04:19:27 2018

video.Capability: Fix dynamically detect capability cases

I pass info into autocaps.Read() arguments in capability.go. It leads to not
dynamically detect keplers or processors. I should be pass "nil" intead.

BUG= chromium:899595 
TEST=video.Capability on guado

Change-Id: I458aeaf53ab198f31b1f8d97059c1b832ef7df38
Reviewed-on: https://chromium-review.googlesource.com/1317219
Commit-Ready: Hirokazu Honda <hiroh@chromium.org>
Tested-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/233aa756261306f0d22d37b29ec94d63366a0abb/src/chromiumos/tast/local/bundles/cros/video/capability.go

tast.video.Capability is always failing on betty-release because autotests-capability says usb_camera is available but it's unavailable.
e.g. https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/buildDetails?buildbucketId=8930492718746004832

We added usb_camera capability in betty's autotest-capability to test camera feature in VM. (cf. Issue 852302)
But before we use a camera there, we always need to enable it by modprobe.
e.g. http://cs/chromeos_public/src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/video/webrtc/webrtc.go?l=54&rcl=a8452c269c26dee0d76412744c8e52a121cf242c

I think we have two options:
1. Don't run tast.video.Capability on VM boards.
2. When we run this test on VM, enable vivid at first like tast.video.WebRTC*
<UI triage> Bug owners, please add the appropriate component to your bug. Thanks!
<UI triage> Bug owners, please add the appropriate component to your bug. Thanks!
keeichiw@, thanks for finding out them.

I would like to keep running this test on VM to guarantee our settings are correct and the tests (don't) run following the settings on VM.
I would like to fix the test by enabling it by modprobe.

I think we should have the code only for initial setup (e.g. init.go) like that in video directory so that we do the common initialization always in all the tests rather than initializing some specific setup for each test case?

derat@, keiichiw@, nya@, what do you think?

Cc: hidehiko@chromium.org
Components: OS>Kernel>Video Tests>Tast
To make sure I understand: are you suggesting that Tast should have framework-level support for performing setup for each category of tests (e.g. video, ui, etc.)?

If so, I don't think we should do this. The number of tests in each category grows over time, and it's unlikely that requirements will be shared across all tests like this. It's important to avoid doing unnecessary work, because that leads to slowness.

Doing this would also make tests harder to read, as there would be code that runs because of the test but that isn't visible when reading the test code. See e.g. the "Avoid magic" point under http://go/tast-design#The-test-framework_and-tests-themselves_should-be-maintainable .

Does it make sense to unconditionally load the vivid module on betty? If so, that can probably just be done by modifying the betty board overlay.

Otherwise, I'd recommend adding an e.g. ensureCameraReady function to a subpackage under the video directory that tests can call that will load vivid if its needed.

Comment 11 Deleted

It might be a good idea to enable vivid by default on VM boards (betty and amd64-generic).
But, we might need some investigation for it as Doug suggested in crbug.com/852302#c40.
Doug, what should we check for vivid?

And, I wonder if cros-camera's changes will be needed if we enable it in overlay. Though vivid is regarded as an external USB camera now, what happens if we enable vivid by default?
Doug was referring to enabling the compilation in standard config, shared by all (or all x86) boards. Since we already have the module behind a USE flag and are building and installing it on the VM boards already, loading it shouldn't hurt, especially since those are not production boards.
Ah, I misunderstood his comment. Thanks Tomasz for correcting me. 
Then, it would be good from this point of view.

So, my only concern is cros-camera's test coverage.
For example, even if we change the way of loading vivid, can we still exercise the cros-camamera's functionality for using newly detected external cameras?
If so, I can support this way.

Ricky, Shik, how do you think?
cros-camera will still treat vivid as an external camera even if we enable vivid on VM by default. Only cameras listed in /etc/camera/camera_characteristics.conf will be regarded as built-in cameras.
Thanks Shik!
(I just remembered that I have asked the same question to you before:P)

Then, I think we can load vivid unconditionally on amd64-generic and betty.
We don't need to modify camera tests in advance because nothing will happen when modprobe is called multiple times.

Does anyone know which file we should add modprobe command in?
You can have a /etc/init/vivid.conf upstart script to load the vivid module. Whether to install the vivid.conf file can be controlled with the same USE flag that controls module.
I'm fine w/ an upstart script if that's what you want, but one thing that might be simpler is to just make this a kernel builtin rather than a module.  This USE flag is only enabled on VMs.  This will make the kernel slightly bigger and could affect boot time, but maybe we don't care for VMs?

...anyway, up to you guys.
Blockedon: 904730
Thanks. I created  Issue 904730  to track the progress for enabling vivid on VM by default.
Project Member

Comment 22 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/b0728f91ef19cf61816f028b1a550749eb9944b6

commit b0728f91ef19cf61816f028b1a550749eb9944b6
Author: Keiichi Watanabe <keiichiw@chromium.org>
Date: Wed Nov 21 02:27:25 2018

chromeos-init: Install vivid upstart config

If USE flag 'vivid' is enabled, install vivid.conf to /etc/init/,
which loads vivid as a video capture device.

BUG= chromium:904730 ,  chromium:899595 
TEST=Made sure that /dev/video0 exists right after rebooting amd64-generic VM
TEST=Made sure that /etc/init/vivid.conf doesn't exist on kevin
TEST=tast run video.Capability on amd64-generic VM right after rebooting
TEST=tast run video.WebRTCCamera on amd64-generic VM
CQ-DEPEND=CL:1333099

Change-Id: I2a22e885af5ce98ea80fe661faa21d7e13ea1ec5
Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1333102
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>

[modify] https://crrev.com/b0728f91ef19cf61816f028b1a550749eb9944b6/chromeos-base/chromeos-init/chromeos-init-9999.ebuild

Project Member

Comment 23 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/05027941ad0a7a492e332c2c62999d41215efcaf

commit 05027941ad0a7a492e332c2c62999d41215efcaf
Author: Keiichi Watanabe <keiichiw@chromium.org>
Date: Wed Nov 21 02:27:25 2018

video.WebRTC*: Don't load/unload vivid in Tast tests

Since vivid is loaded by upstart service on VM boards, each test
case doesn't have to load/unload it any more.

But, we still need to check if the device is VM or not. It's because
we check the ratio of broken frames when the used camera is vivid,
which doesn't have hardware flakiness.

BUG= chromium:904730 ,  chromium:899595 
TEST=tast run video.WebRTCCamera on amd64-generic VM
TEST=tast run video.Capability after running WebRTC tests on VM
CQ-DEPEND=CL:1333099, CL:1333102

Change-Id: I2e794a72dc1f826f887c8489a03c7a5206f1226c
Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1335058
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/05027941ad0a7a492e332c2c62999d41215efcaf/src/chromiumos/tast/local/bundles/cros/video/webrtc/webrtc.go

Project Member

Comment 24 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/cf30ae11494b102f27ffdec4d803abee6d05753f

commit cf30ae11494b102f27ffdec4d803abee6d05753f
Author: Keiichi Watanabe <keiichiw@chromium.org>
Date: Wed Nov 21 02:27:24 2018

init: Add vivid.conf

Add config file to load vivid at start-up time.

BUG= chromium:904730 ,  chromium:899595 
TEST=Made sure that vivid was loaded after rebooting amd64-generic VM
     (with CL:1333102)
Change-Id: Ice02e2d93ccb858fb1098c6ae00ea50323390be6
Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1333099
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>

[add] https://crrev.com/cf30ae11494b102f27ffdec4d803abee6d05753f/init/upstart/vivid/vivid.conf

Sign in to add a comment