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

Issue 735783 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

VDA unittest failing on kepler: num_decoded_frames not enough

Project Member Reported by wuchengli@chromium.org, Jun 22 2017

Issue description

This is a known issue. go2001 didn't implement flush and the decoder was not fast enough. The number of decoded frames was lower than expected. This is failing 100% on buddy and rikku.

06/21 15:24:26.077 DEBUG|             utils:0280| [stdout] ../../../../../../../home/chrome-bot/chrome_root/src/media/gpu/video_decode_accelerator_unittest.cc:1516: Failure
06/21 15:24:26.078 DEBUG|             utils:0280| [stdout]       Expected: client->num_decoded_frames()
06/21 15:24:26.078 DEBUG|             utils:0280| [stdout]       Which is: 242
06/21 15:24:26.078 DEBUG|             utils:0280| [stdout] To be equal to: video_file->num_frames
06/21 15:24:26.078 DEBUG|             utils:0280| [stdout]       Which is: 250
 
log.txt
5.5 KB View Download
Blockedon: 674791
This will be fixed once  issue 674791  is implemented.
Correction. The failure rate is very low. Maybe just 5%.
Labels: videoshortlist
Owner: hiroh@chromium.org
Status: Assigned (was: Available)
Cc: owenlin@chromium.org
 Issue 740797  has been merged into this issue.

Comment 6 by hiroh@chromium.org, Jul 27 2017

Status: Started (was: Assigned)

Comment 7 by hiroh@chromium.org, Jul 28 2017

Blockedon: -674791

Comment 8 by hiroh@chromium.org, Jul 28 2017

Cc: tfiga@chromium.org
This test was failed because a resolution change event is not processed correctly in V4L2VDA.
There are two kind of resolution change event: initial one and dynamic one.
A driver sends an initial resolution change event if it obtains enough data for allocating CAPTURE queue. This is an initial resolution change event.
Dynamic resolution change event is, just like the name, one dynamically changed during streaming.
V4L2VDA has two locations to process resolution change events: BufferInitial() for initial ones and ServiceDeviceTask() for dynamic ones.
The bug reason is due to race of these functions.
I changed V4L2VDA as these resolution change events are processed correctly.
Code review is here: crrev.com/c/588034.

mfc and mtk both do not send an initial resolution change event, which does not follow V4L2 specification. tomasz@ wrote patches to fix it: crrev.com/c/588878 and crrev.com/c/589087.

Comment 9 by hiroh@chromium.org, Aug 1 2017

We generally need to have backward compatibility support; chrome has to work if a driver is not updated.
In this case, VDA should work on a device whose driver does not send an initial resolution change event.
Therefore, I changed as VDA works on such environment.
As a result, the chrome patch (crrev.com/c/588034) can be merged without breaking VDA unittest if the driver patches are not merged (crrev.com/c/588878 and crrev.com/c589087).

Comment 10 by hiroh@chromium.org, Sep 26 2017

I tested the CL (crrev.com/c/588034) by CTS test on elm and EOSBehavior tests were failed due to it.
Apart from that, some CTS tests has been failed on elm.
So I am going to fix other CTS tests on elm first and thereafter change this CL not to fail CTS tests.
That will probably be next week.

Comment 11 by hiroh@chromium.org, Sep 26 2017

Because the reproduce rate of failures of other CTS tests was quite lower on my local machine (<5%) than on kcwu@'s local machine (about 80%), it seems to be better that kcwu@ investigates the task.
So I try to fix the failure of the CTS test due to crrev.com/c/588034.
The exact CTS test names which get failed by the CL are  android.media.cts.DecoderTest#testEOSBehavior(H264/VP8/VP9).
I investigated the failure reason today.
The test issues Initialize, Decode, Decode, Decode, Flush, (Reset) in this order.
V4L2VDA state is changed from kInitialize to kStartResolutionChange if an initial resolution change starts.
All the kind of resolution changes are processed in ServiceDeviceTask() in this CL.
ServiceDeviceTask() are invoked by Initialize(), but the execution will be after Decode() and Flush().
The point is Flush() just returns and invoke NotifyFlushDone(), although decoding previous frames are not done.
This is because VDA status are kInitialize since an initial resolution change is not caught and processed yet.
However, EOSBehavior test seems to assume all the transferring buffers are done if NotifyFlushDone() is executed.

I couldn't decide how I should fix, fixing CTS test or fixing Chromium V4L2VDA workflow.
I will make a decision with persons who know better.

Comment 12 by hiroh@chromium.org, Oct 20 2017

I tried to fix by merely doing Flush during Initialization.
Since Flush queues a flush buffer to Input buffer, queued buffer before the flush buffer would be decoded after initialization.
So it seemed to be no problem and should work.
However, although testEOSBehavior(H264/VP8/VP9) got passed, some tests became newly failed.

Comment 13 by hiroh@chromium.org, Oct 23 2017

The example of newly failed tests is  android.media.cts.DecoderTest#testVP8Decode30fps1280x720.
The tests are failed by "fail: junit.framework.AssertionFailedError: last frame didn't have EOS."
They fails with low probability.
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 19 2017

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

commit e8a2b31934080568a52ca473aa2c5a2880e02438
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Tue Dec 19 09:00:31 2017

V4L2VDA: fix initial resolution change handling

The V4L2 driver should send a resolution change event once enough data is found
in the stream to infer the coded size. Some legacy drivers do not support
the resolution change event however, and, to support them, V4L2VDA,
in addition to handling resolution change events, also explicitly queries for
current format after each decode run, to see if it changed.
This may however cause the resolution change to be processed twice for drivers
that do send an event: once as a result of receiving the event,
and once after explicitly querying for the format. Fix this by
explicitly dequeuing any pending events after detecting format change
in the latter case.

BUG= chromium:735783 
TEST=VDA unittest on buddy,hana and CTS test on hana

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I393c849b0355ff67a14e8ad21f29451ca70efce2
Reviewed-on: https://chromium-review.googlesource.com/588034
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524973}
[modify] https://crrev.com/e8a2b31934080568a52ca473aa2c5a2880e02438/media/gpu/v4l2/v4l2_video_decode_accelerator.cc

Comment 15 by hiroh@chromium.org, Dec 19 2017

I consider the above CL will solve the bug.
Since it changes V4L2VDA code, I am going to carefully monitor CTS tests and ChromeOS video tests in few days from when the CL will be landed on ChromeOS ToT.
looks like video_VDAPerf on buddy and rikku starts to be passed on buddy and rikku. 
video_VDAPerf fails one time on nyan_big by the same reason.

The failure on nyan_big couldn't be reproduced, while I run the VDA unittest 500 times.
Project Member

Comment 18 by bugdroid1@chromium.org, Jan 9 2018

Labels: merge-merged-chromeos-3.18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/26ce30d7d3c128f760e80df3d0feb35af9fe63a7

commit 26ce30d7d3c128f760e80df3d0feb35af9fe63a7
Author: Tomasz Figa <tfiga@chromium.org>
Date: Tue Jan 09 19:54:54 2018

FROMLIST: media: mtk-vcodec: Always signal source change event on format change

Currently the driver signals the source change event only in case of
a midstream resolution change, however the initial format detection
is also defined as a source change by the V4L2 codec API specification.
Fix this by signaling the event after the initial header is parsed as
well.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
(am from https://patchwork.linuxtv.org/patch/46370/)

BUG= chromium:735783 
TEST=video_decode_accelerator_unittest

Change-Id: Id4f3c719953af7c74c94f48177159189cc05db0f
Reviewed-on: https://chromium-review.googlesource.com/588878
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Wu-Cheng Li <wuchengli@chromium.org>

[modify] https://crrev.com/26ce30d7d3c128f760e80df3d0feb35af9fe63a7/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c

Comment 19 by hiroh@chromium.org, Jan 10 2018

Cc: kcwu@chromium.org
Some test cases in CTS Media Test start to fail with law probability on elm after this CL is merged.
For example, android.media.cts.DecoderTest#testVP8Decode320x180.

I don't consider this CL is reverted, and instead solve it by a fix CL.

Comment 20 by kcwu@chromium.org, Jan 10 2018

I assume you are referring to the CL in comment 18 as "this CL", right?

Comment 21 Deleted

Comment 22 by hiroh@chromium.org, Jan 10 2018

> I assume you are referring to the CL in comment 18 as "this CL", right?
No. I referred the CL in comment 14. (Sorry for confusing.)


I run CTS test on hana some times and verified it.
However, the truth is a test fails with law probability.
Project Member

Comment 23 by bugdroid1@chromium.org, Jan 12 2018

Labels: merge-merged-chromeos-3.8
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/7b891098f189dba65ac8563203782260dca5fafd

commit 7b891098f189dba65ac8563203782260dca5fafd
Author: Tomasz Figa <tfiga@chromium.org>
Date: Fri Jan 12 13:49:43 2018

CHROMIUM: [media] s5p-mfc: Always signal source change event on format change

Currently the driver signals the source change event only in case of
a midstream resolution change, however the initial format detection
is also defined as a source change by the V4L2 codec API specification.
Fix this by signaling the event after the initial header is parsed as
well.

Also, remove the legacy behavior of VIDIOC_G_FMT, which used to
synchronously wait for hardware to finish parsing the header, because it
is against the V4L2 codec API specification and can actually deadlock
the userspace in case when an OUTPUT buffer that does not contain
headers is queued.

BUG= chromium:735783 
TEST=video_decode_accelerator_unittest

Change-Id: I391c2ba28c2a14420332de3578e39511976d7e49
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/589087
Tested-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>

[modify] https://crrev.com/7b891098f189dba65ac8563203782260dca5fafd/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c

Comment 24 by hiroh@chromium.org, Jan 19 2018

Status: Verified (was: Started)
The tests are passed in buddy and rikku.
Additionally, tests on other boards are still passed after the driver changes #c18 and #c23.
It is time now to mark verified.

Sign in to add a comment