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

Issue 833698 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature



Sign in to add a comment

Add more test cases to VDA unittest

Project Member Reported by hiroh@chromium.org, Apr 17 2018

Issue description

VDA unittest doesn't have sufficient Flush() test cases.
We need to add more test cases about timing Flush() and confirm VDA
surely returns NotifyFlushDone() for any Flush() timing.

Note: VDA client can Flush() and VDA should eventually invoke NotifyFlushDone().

Changing VDA unittest will affect on its result on other platforms, e.g., Android, Mac and Windows.
It is needed to verify on them as well. Perhaps, at first, we will not test additional test cases on some of them.
 
Labels: -Pri-2 Pri-1
It's ok to enable the new test case for ChromeOS first.

Comment 2 by hiroh@chromium.org, Apr 18 2018

Do you mean new autotest or adding to Chrome VDA unittest?
After you add new test case in VDA unittest, trybot can test on other platforms like Mac and Windows. If you find their VDA implementation cannot pass the new test case, you can add a #ifdef CHROMEOS so the new test case only runs on ChromeOS.

Comment 4 by hiroh@chromium.org, Apr 24 2018

Summary: Add more test cases to VDA unittest (was: Add Flush() test cases to VDA unittest)
1. medium Flush() and wait NotifyFlushDone()
2. mix Flush and Reset()
3. Resolution change (video specific)

Comment 5 by hiroh@chromium.org, Apr 24 2018

4. add no Reset() test case

Comment 6 by hiroh@chromium.org, May 9 2018

Status: Started (was: Assigned)

Comment 7 by hiroh@chromium.org, May 10 2018

The CL for enabling resolution change tests was landed.
https://chromium-review.googlesource.com/c/chromium/src/+/1027690

Comment 8 by tfiga@chromium.org, May 28 2018

Just a random observation:

Looks like after the CL mentioned in #7, we don't validate the resolutions that come from the kernel anymore, since we just compare framebuffer size with visible size, both coming from the kernel.

I think it wouldn't be a bad idea to have resolution changes listed as a set of <frame number, resolution> and check if visible size matches respective resolution. (Framebuffer size shouldn't be asserted, since driver might round it up according to hardware requirements. The existing framebuffer_size >= visible_size check should be enough.)
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 27 2018

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

commit 2d07b945827a2af45db4f99766cd6ccadd94aea2
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed Jun 27 02:56:28 2018

media/gpu VDA unittest: Delete VDA properly in TearDownTiming case

Since VDA unittest doesn't manage properly the number of play throughs, VDA
(i.e. decoder) is not deleted at the specified timing in TearDownTiming case.
This corrects the management of play though times to make TearDownTiming case
the intended behavior.
TearDownTimingCase in CS_RESET is removed because the case is already covered by
DecodeVarations/0 test case.

BUG= chromium:833698 ,  chromium:834170 
TEST=VDA unittest on eve, hana, kevin

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I9ef0ab3d4a7b80f022b9c73befeb561655c6b2fc
Reviewed-on: https://chromium-review.googlesource.com/1102224
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570646}
[modify] https://crrev.com/2d07b945827a2af45db4f99766cd6ccadd94aea2/media/gpu/video_decode_accelerator_unittest.cc

Status: Fixed (was: Started)
We will add more test cases in a new test code developed by dstaessens@.
So I close this issue.

Sign in to add a comment