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

Issue 834170 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Clean up VDA unittest

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

Issue description

VDA unittest code is messy. I will do things to clean up the test. 
This is the issue tracker for this task.
 

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

Create the following three CLs as a first step.
https://chromium-review.googlesource.com/c/chromium/src/+/1016222
https://chromium-review.googlesource.com/c/chromium/src/+/1016443
https://chromium-review.googlesource.com/c/chromium/src/+/1016822

The main purpose of them is to reduce the amount of code in video_decode_accelerator_unittest.cc, so that we can more focus on VDAClient in the file.

Next, I plan to create VDAClientBase.
VDAClientBase implements typical VDA::Client functions for test.
VDAClients in VDA_unittest.cc can reuse VDAClientBase functions.
It would be complex a bit and thus I will write a brief design doc first.

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

I experimentally implement VDAClientBase and handle Reset test cases using VDAClientResetTestClient that inherits VDAClientBase.
This is the experimental CL, crrev.com/c/1025236.
The amount of codes doesn't decrease as much as I expected.
I would rather the existing GLRenderingVDAClient than splitting VDAClientBase.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 26 2018

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

commit e72ec235e54f426426735423eb4cf5ea1fff0802
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu Apr 26 05:09:18 2018

media/gpu: move test related files to media/gpu/test data directory

BUG= chromium:834170 
TEST=VDA/VEA/JDA/JEA unittest built scueessfully for Chrome OS on both x86 and
ARM platforms
TEST=made sure for other platforms by CQ

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: I480220047ac23e4c26ee4dfff0ac2ef3bac70f6b
Reviewed-on: https://chromium-review.googlesource.com/1016222
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553924}
[modify] https://crrev.com/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/BUILD.gn
[modify] https://crrev.com/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/jpeg_decode_accelerator_unittest.cc
[modify] https://crrev.com/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/jpeg_encode_accelerator_unittest.cc
[rename] https://crrev.com/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/test/rendering_helper.cc
[rename] https://crrev.com/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/test/rendering_helper.h
[rename] https://crrev.com/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/test/video_accelerator_unittest_helpers.h
[modify] https://crrev.com/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/video_decode_accelerator_unittest.cc
[modify] https://crrev.com/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/video_encode_accelerator_unittest.cc

Components: OS>Kernel>Video
Additional suggestion: base::MemoryMappedFile may be more efficient for especially larger streams (vs. using std::string).
Project Member

Comment 5 by bugdroid1@chromium.org, May 1 2018

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

commit e56443b09876cc2889e76da590e2460865bf6a62
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Tue May 01 20:28:36 2018

media/gpu/test VDA: Move some classes and functions from unittest to helper file

Move VideoDecodeAcceleratorTestEnvironment and TextureRef to VDA unittest heler
in order to reduce the amount of code in VDAunittest.cc.

BUG= chromium:834170 
TEST=VDA unittest on kevin and caroline
TEST=VDA unittest on other platforms in CQ

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: Ib7912e98a7cc0c07c8a343f512094b7b2ba6bfa8
Reviewed-on: https://chromium-review.googlesource.com/1016443
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555170}
[modify] https://crrev.com/e56443b09876cc2889e76da590e2460865bf6a62/media/gpu/BUILD.gn
[add] https://crrev.com/e56443b09876cc2889e76da590e2460865bf6a62/media/gpu/test/video_decode_accelerator_unittest_helpers.cc
[add] https://crrev.com/e56443b09876cc2889e76da590e2460865bf6a62/media/gpu/test/video_decode_accelerator_unittest_helpers.h
[modify] https://crrev.com/e56443b09876cc2889e76da590e2460865bf6a62/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, May 1 2018

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

commit 6d9f8a44b4bce6edb29911783ccd2fa5e415d8af
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Tue May 01 20:43:58 2018

media/gpu/test VDA: Implement EncodedDataHelper to handle input (encoded) data

GLRenderingVDAClient has several functions to handle input data (i.e. encoded
data), e.g., GetBytesForFirstFragment() and GetBytesNextNALU().
They are not core role of GLRenderingVdaClient.
This introduces EncodedDataHelper as a VDA unittest helper class. It handles
encoded data instead of GLRenderingVDAClient.

BUG= chromium:834170 
TEST=VDA unittest on kevin and caroline
TEST=VDA unittest on other platforms in CQ

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: I5116b03172b6a7a80b0473d75d4aeb23868db20d
Reviewed-on: https://chromium-review.googlesource.com/1016822
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555176}
[modify] https://crrev.com/6d9f8a44b4bce6edb29911783ccd2fa5e415d8af/media/gpu/test/video_decode_accelerator_unittest_helpers.cc
[modify] https://crrev.com/6d9f8a44b4bce6edb29911783ccd2fa5e415d8af/media/gpu/test/video_decode_accelerator_unittest_helpers.h
[modify] https://crrev.com/6d9f8a44b4bce6edb29911783ccd2fa5e415d8af/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, May 2 2018

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

commit 1377f7c7e9f4ef1ba9b6db3791c78f85b108f6d0
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed May 02 00:46:12 2018

media/gpu VDA unittest: Simplify Reset test case workflow

Originally, it was hard to understand Reset test case workflow, since a variable
named reset_after_frame_num can mean either ResetPoint or the number of frames.
This simplifies the workflow by introducing a variable to describe ResetPoint,
so that reset_after_frame_num can only be meaningful for MID_STREAM_RESET.

BUG= chromium:834170 
TEST=VDA unittest on eve and kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: I5c0b5a6b3ea99c98f54eb244a6fed5059f77c102
Reviewed-on: https://chromium-review.googlesource.com/1025531
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555259}
[modify] https://crrev.com/1377f7c7e9f4ef1ba9b6db3791c78f85b108f6d0/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, May 2 2018

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

commit 84bfafcfc900120a837a023924711ab5a18afb6a
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed May 02 00:49:58 2018

media/gpu VDA unittest: Refactor thumbnail validity check part

Move Convert from RGBA to RGB part into helper files.
Change the function to read known md5s and use base::ContainsValue to verify it.

BUG= chromium:834170 
TEST=VDA unittest on eve and kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: Ic965ee1668af67ecd04956bc89a29bf7a7de5f84
Reviewed-on: https://chromium-review.googlesource.com/1025535
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555262}
[modify] https://crrev.com/84bfafcfc900120a837a023924711ab5a18afb6a/media/gpu/test/video_decode_accelerator_unittest_helpers.cc
[modify] https://crrev.com/84bfafcfc900120a837a023924711ab5a18afb6a/media/gpu/test/video_decode_accelerator_unittest_helpers.h
[modify] https://crrev.com/84bfafcfc900120a837a023924711ab5a18afb6a/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, May 2 2018

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

commit 64c218a5099768a71c4739c357593694dd5ceb5e
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed May 02 00:58:03 2018

media/gpu VDA unittest: Move constant values and enum to top of the file

In additon to moving those declaration places, changes the type of
kMinSupportedNumConcurrentDecoders to const size_t from enum.

BUG= chromium:834170 
TEST=VDA unittest on eve and kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: I5b343adb64811f8f6dde2699af60542c666890af
Reviewed-on: https://chromium-review.googlesource.com/1027333
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555265}
[modify] https://crrev.com/64c218a5099768a71c4739c357593694dd5ceb5e/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, May 2 2018

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

commit 4a3688cf3c45e4d4a4fc88609430109615cc099a
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed May 02 01:58:45 2018

media/gpu VDA unittest: Pass the initialization arguments with Config

Introduce GLRenderingVDAClient::Config and pass arguments to
GLRenderingVDAClient's constructor with it.

BUG= chromium:834170 
TEST=VDA unittest on eve and kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: I5db1cbd9140f9a347f45f89b3903b3f1334b36e1
Reviewed-on: https://chromium-review.googlesource.com/1027490
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555285}
[modify] https://crrev.com/4a3688cf3c45e4d4a4fc88609430109615cc099a/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, May 9 2018

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

commit c58ee940e1df9e382fa849247e397767fcd5afda
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed May 09 04:24:24 2018

media/gpu VDA unittest: Sort up variable types

This changes some variable types to more proper types. For example, int to
size_t, bool, uint32_t or double.

BUG= chromium:834170 
TEST=VDA unittest on kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: Ie41977c56edeb800fb2938d20fe0ed371fdfb131
Reviewed-on: https://chromium-review.googlesource.com/1039206
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557090}
[modify] https://crrev.com/c58ee940e1df9e382fa849247e397767fcd5afda/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, May 9 2018

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

commit e885c34b7374e801895fb0adc47f4ed6e2fc07b8
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed May 09 09:04:17 2018

media/gpu VDA unittest: Always output thumbnails in thumbnail testcase

Always output thumbnails in thumbnail testcase. the output thumbanils file name
contains "good" if the thumbnail test case is passed, and on failure it contains
"bad."

BUG= chromium:834170 
TEST=VDA unittest on kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: I428793cd6428420495be9a3ab60170669e5f51fe
Reviewed-on: https://chromium-review.googlesource.com/1039104
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557128}
[modify] https://crrev.com/e885c34b7374e801895fb0adc47f4ed6e2fc07b8/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, May 10 2018

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

commit 7c509ec6f440a367a7779fe0f1a7d021c8889305
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu May 10 02:01:54 2018

media/gpu VDA unittest: Enable testing resolution change videos

In VDA unittest, the video frame size is given as input, and the unittest checks
that the visible size does not change.
To enable resolution changing videos, set the frame size dynamically when a
resolution change happens (i.e. ProvidePictureBuffers()).

BUG= chromium:834170 
TEST= on kevin
$ ./video_decode_accelerator_unittest --test_video_data=switch_720p_1080p_240frames.h264:1920:1080:240:240:24:24:1 --ozone-platform=gbm
$ ./video_decode_accelerator_unittest --test_video_data=resolution_change_500frames-vp8.ivf:640:360:500:500:30:30:11 --ozone-platform=gbm
$ ./video_decode_accelerator_unittest --test_video_data=resolution_change_500frames-vp9.ivf:640:360:500:500:30:30:12 --ozone-platform=gbm


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: I803afb16bc3a706ea9f806ac1f1372c24f80c341
Reviewed-on: https://chromium-review.googlesource.com/1027690
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557422}
[modify] https://crrev.com/7c509ec6f440a367a7779fe0f1a7d021c8889305/media/gpu/video_decode_accelerator_unittest.cc
[modify] https://crrev.com/7c509ec6f440a367a7779fe0f1a7d021c8889305/media/test/data/README
[add] https://crrev.com/7c509ec6f440a367a7779fe0f1a7d021c8889305/media/test/data/resolution_change_500frames-vp8.ivf
[add] https://crrev.com/7c509ec6f440a367a7779fe0f1a7d021c8889305/media/test/data/resolution_change_500frames-vp9.ivf
[add] https://crrev.com/7c509ec6f440a367a7779fe0f1a7d021c8889305/media/test/data/switch_1080p_720p_240frames.h264

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 13 2018

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

commit 3ffda9ea5e7f142cabc1f354ebb6ea6b79b419e0
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed Jun 13 00:36:37 2018

media/gpu VDA unittest: Do not call ReusePictureBuffer on a dimissed picture buffer

A VDA client should not ReusePictureBuffer() on a dismissed picture buffer.
VDA doesn't have to handle this case.

BUG= chromium:834170 
TEST=VDA unittest on eve and kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: Ia3bc2534684b5448c01fdc86d7efc7db4389c21c
Reviewed-on: https://chromium-review.googlesource.com/1063890
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566655}
[modify] https://crrev.com/3ffda9ea5e7f142cabc1f354ebb6ea6b79b419e0/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Jun 13 2018

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

commit 3ffda9ea5e7f142cabc1f354ebb6ea6b79b419e0
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed Jun 13 00:36:37 2018

media/gpu VDA unittest: Do not call ReusePictureBuffer on a dimissed picture buffer

A VDA client should not ReusePictureBuffer() on a dismissed picture buffer.
VDA doesn't have to handle this case.

BUG= chromium:834170 
TEST=VDA unittest on eve and kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: Ia3bc2534684b5448c01fdc86d7efc7db4389c21c
Reviewed-on: https://chromium-review.googlesource.com/1063890
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566655}
[modify] https://crrev.com/3ffda9ea5e7f142cabc1f354ebb6ea6b79b419e0/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Jun 13 2018

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

commit e43820dd74b64e428e17fe9688fe463b53882a09
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed Jun 13 03:17:26 2018

media/gpu VDA unittest: Check the number of decoded frames in NotifyFlushDone()

The number of decoded frames should be equal to the number of frames of input
video, if no Reset() is invoked before Flush().
Should check this condition in VDA unittest.

BUG= chromium:834170 
TEST=VDA unittest on eve and kevin
TEST=VDA unittest on non-Chrome OS platforms in CQ.

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: I2eda613522b9a1e6259a9d72c4f86b616bc90996
Reviewed-on: https://chromium-review.googlesource.com/1063910
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566703}
[modify] https://crrev.com/e43820dd74b64e428e17fe9688fe463b53882a09/media/gpu/video_decode_accelerator_unittest.cc

Comment 17 by hiroh@chromium.org, Jun 13 2018

VDA unittest has a bug in TearDownTiming case. It doesn't actually tear down.
For example, delete_decoder_state is CS_DECODER_SET, in other words, a decoder is intended to be deleted after it is created.
Looks like VDA unittest expects to delete a decoder in SetState(CS_DECODER_SET).
https://chromium.googlesource.com/chromium/src/+/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/video_decode_accelerator_unittest.cc#911
When reaching the line, because remaining_play_throughs_ is 1, the condition is false. So DeleteDeocder() is not executed.
As a result, decoder is eventually deleted in NotifyResetDone().
https://chromium.googlesource.com/chromium/src/+/e72ec235e54f426426735423eb4cf5ea1fff0802/media/gpu/video_decode_accelerator_unittest.cc#882

The above code is one I made my changes.
Although I don't know when this bug exists in VDA unittest, I have to fix this ASAP.

Project Member

Comment 18 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

Project Member

Comment 19 by bugdroid1@chromium.org, Jun 28 2018

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

commit cdd5b7704813bed42deeb83b03c194520f0f8334
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu Jun 28 08:40:27 2018

media/gpu/VDA unittest: Don't check state_ in PictureReady() if VDA is deleted.

By crrev.com/c/1102224, VDA is actually deleted in TearDownTiming test case.
Before deleting decoder and VDA client's state becomes CS_DESTROYED, VDA can
invoke PictureReady and those are processed after DeleteDecoder(). At that time,
state is CS_DESTROYED and thus LOG_ASSERT(state < CS_RESET) throws an exception.
The LOG_ASSERT should be executed only if VDA is not deleted.

BUG= chromium:834170 
TEST=VDA unittest at veyron_minnie

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: I1d18ca4f48f6c0e7129f7364e8d7b0ea083b513f
Reviewed-on: https://chromium-review.googlesource.com/1118081
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571047}
[modify] https://crrev.com/cdd5b7704813bed42deeb83b03c194520f0f8334/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Jul 6

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

commit 1f438546b68f19d9cc8a377c9ae789d4f09933ef
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Fri Jul 06 04:08:22 2018

media/gpu/VDA unittest: Always remove TextureRef in ReturnPicture()

TextureRef should be removed always in ReturnPicture(). Otherwise, if a decoder
is deleted, a TextureRef in pending_textures_ is possibly not released until
GLRenderingVDAClient.

BUG= chromium:834170 
TEST=VDA unittest at kevin and eve

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: Ibc63f0bf751cf5cb011102ba7fa6ae629ad841e7
Reviewed-on: https://chromium-review.googlesource.com/1127208
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572889}
[modify] https://crrev.com/1f438546b68f19d9cc8a377c9ae789d4f09933ef/media/gpu/video_decode_accelerator_unittest.cc

We need to rethink the existing ASSERT/EXPECT are reasonable and must change CHECK to ASSERT/EXPECT. cf.) go/totw/113
Project Member

Comment 22 by bugdroid1@chromium.org, Jul 12

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

commit c9e1a62b97488da8c36e7c12d0fe9769a60b4238
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu Jul 12 06:09:20 2018

media/gpu/test/RenderingHelper: Unify interface to process video frame

VDA test client calls RenderingHelper::RenderThumbnail() if a thumbnail is made
from a video frame. Otherwise, it calls RenderingHelper::QueueVideoFrame().
RenderingHelper itself knows which function to be called, and thus these
functions should be unified into a single interface.

BUG= chromium:834170 
TEST=VDA unittest at veyron_minnie and eve

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: I106d4ce1c05b003d6333a6865a8d96e276031598
Reviewed-on: https://chromium-review.googlesource.com/1122656
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574496}
[modify] https://crrev.com/c9e1a62b97488da8c36e7c12d0fe9769a60b4238/media/gpu/test/rendering_helper.cc
[modify] https://crrev.com/c9e1a62b97488da8c36e7c12d0fe9769a60b4238/media/gpu/test/rendering_helper.h
[modify] https://crrev.com/c9e1a62b97488da8c36e7c12d0fe9769a60b4238/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Jul 18

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

commit 538a599cd410a752e858a9bc593ee75d90977cb9
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed Jul 18 13:25:54 2018

media/gpu/VDA unittest: Check if requested number Decode()s is called in flight

This adds the checker that the number of Decode()s in flight is less than or
equal to the requested number.
In addition, by this change,
(1) if a decoder is deleted, doesn't execute DecodeNextFragment() and doesn't
check the number of Decode()s,
(2) doesn't DecodeNextFragment() in NotifyResetDone() caused by
MID_STREAM_RESET or RESET_AFTER_FIRST_CONFIG_INFO, because requested number
Decode()s are being executed.

BUG= chromium:834170 
TEST=VDA unittest at 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: Icdb94ff57b824690cf8346e7d9c6889ba57ae0d3
Reviewed-on: https://chromium-review.googlesource.com/1130970
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576019}
[modify] https://crrev.com/538a599cd410a752e858a9bc593ee75d90977cb9/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Aug 6

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

commit 0f121c9f1bad4c258d5e3678a1eb9c1ca8a6d693
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Mon Aug 06 14:45:35 2018

media/gpu/test/RenderingHelper: Create TextureRef in CreateTexture()

RenderingHelper::CreateTextureRef should encapsulate creating TextureRef, not
only creating texture id. Thanks to this, RenderingHelper don't need to
publicize a function to close a created texture.

BUG= chromium:834170 
TEST=VDA unittest on veyron_minnie and eve

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: I21db66ef8505bd5ae67c0546ed2fd84890b011eb
Reviewed-on: https://chromium-review.googlesource.com/1122071
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580864}
[modify] https://crrev.com/0f121c9f1bad4c258d5e3678a1eb9c1ca8a6d693/media/gpu/test/rendering_helper.cc
[modify] https://crrev.com/0f121c9f1bad4c258d5e3678a1eb9c1ca8a6d693/media/gpu/test/rendering_helper.h
[modify] https://crrev.com/0f121c9f1bad4c258d5e3678a1eb9c1ca8a6d693/media/gpu/test/video_decode_accelerator_unittest_helpers.cc
[modify] https://crrev.com/0f121c9f1bad4c258d5e3678a1eb9c1ca8a6d693/media/gpu/test/video_decode_accelerator_unittest_helpers.h
[modify] https://crrev.com/0f121c9f1bad4c258d5e3678a1eb9c1ca8a6d693/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Aug 6

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

commit 306ecb4565c96cab55a94c3a4346fdd47494f15f
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Mon Aug 06 15:11:43 2018

media/gpu/test/RenderingHelper: Clean up RenderingHelper code

This CL does
(1) remove unused RenderingHelper::GetGLDisplay(),
(2) move local static functions to noname namespace,
(3) omit RenderingHelperParams' constructors and desturctor.

BUG= chromium:834170 
TEST=VDA unittest at 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: Ibe5b2207c3eb794266f6cbc5d4796debefe165ad
Reviewed-on: https://chromium-review.googlesource.com/1126742
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580872}
[modify] https://crrev.com/306ecb4565c96cab55a94c3a4346fdd47494f15f/media/gpu/test/rendering_helper.cc
[modify] https://crrev.com/306ecb4565c96cab55a94c3a4346fdd47494f15f/media/gpu/test/rendering_helper.h

Status: Fixed (was: Started)
mostly done. Let me mark this Fixed.

Sign in to add a comment