VDA unittest failing on kepler: num_decoded_frames not enough |
|||||||||
Issue descriptionThis 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
,
Jun 22 2017
This will be fixed once issue 674791 is implemented.
,
Jun 22 2017
Correction. The failure rate is very low. Maybe just 5%.
,
Jul 5 2017
,
Jul 13 2017
,
Jul 27 2017
,
Jul 28 2017
,
Jul 28 2017
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.
,
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).
,
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.
,
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.
,
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.
,
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.
,
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
,
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.
,
Jan 9 2018
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.
,
Jan 9 2018
The failure on nyan_big couldn't be reproduced, while I run the VDA unittest 500 times.
,
Jan 9 2018
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
,
Jan 10 2018
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.
,
Jan 10 2018
I assume you are referring to the CL in comment 18 as "this CL", right?
,
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.
,
Jan 12 2018
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
,
Jan 19 2018
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 |
|||||||||
Comment 1 by posciak@chromium.org
, Jun 22 2017