New issue
Advanced search Search tips

Issue 905952 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 879065



Sign in to add a comment

Move the existing VDA flush tests to the new video decode test framework

Project Member Reported by dstaessens@chromium.org, Nov 16

Issue description

The existing HW video decode accelerator flush-related tests in 'video_decode_accelerator_unittest.cc' need to be ported to the new framework defined in: crbug.com/879065
 
Existing test scenarios are defined in: go/VDATestScenario
Blocking: 879065
Labels: OS-Chrome
Labels: -Type-Bug Type-Feature
Status: Available (was: Untriaged)
GPU Triage pass - this looks like feature work, changing to feature and marking available.
Labels: -Type-Feature -OS-Chrome Type-Bug
I'm currently experimenting with the functionality required to implement the flush testcases.

To perform a midstream flush we need to do something like this:
  tvp->Play();
  tvp->WaitForEventCount(VideoPlayerEvent::kFrameDecoded, 10);
  tvp->Flush();

To do so we have multiple options with various pro/contras.

1. Wait for specific frame # decoded event:
 + Easiest to implement
 - Due to the asynchronous nature and multiple in-flight decodes, the flush operation will actually be called a few frames after the specified frame.

2. Wait for specific frame # to be queued for decode (add FrameDecodeQueued event):
 + Would cause the flush operation to be called when we want (might still be very short delay due to asynchronous nature)
 - More difficult to implement as a frame might be made of multiple fragments, and currently there is no functionality to check whether all fragments of a frame have been queued. Some codecs (h264) make this very hard due to frame reordering.

3. Add functionality to check whether a flush should be done, on the decoder client thread itself (e.g. flushPoint = MID_STREAM). This is what the current VDA tests do, directly in the PictureReady callback.
 + We can directly trigger the operation at the intended point.
 - Adding this sort of hardcoded functionality will make the code harder to read and maintain. This is the reason we are rewriting the code, so lets avoid this as much as possible.

4. Add functionality to the play function, to specify up until which point to play. (e.g. PlayUntil(FrameDecoded, 10), PlayUntil(FIRST_CONFIG_INFO))
 + We can trigger operations at the intended point without hardcoding too much.
 - We need to decide what to do with in-flight decodes when that point is reached.

In case of flush testcases, we can decide to live with flush() not always happening at the same location.
In case of reset tests, there is one VDA test that tests resetting immediately after the first config info. 'RESET_AFTER_FIRST_CONFIG_INFO'. This means we would need a way to intercept this.
Cc: posciak@chromium.org
Status: Started (was: Available)
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 10

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

commit 899dda5a9de3455c6d85e25c97e06b5fdf70d6f1
Author: David Staessens <dstaessens@chromium.org>
Date: Mon Dec 10 01:06:17 2018

media/gpu/test: Add flush tests to the new video decode test framework.

This CL adds support for the flush command to the video player, and ports some
of the current VDA flush testscases to the new framework.

Some additional cleanup:
- Added proper state management to the video decoder client, required for flush
  to properly work.
- The test video file is now part of the test environment.
- Renamed some getters, dropped the 'Get' prefix for brevity.
- Added getter for the number of frames in a test video.

TEST=ran new VD tests on nocturne

BUG= 905952 , 905952 

Change-Id: I3f251969bcc015c3e67dd9ae6f5be36edacc5fae
Reviewed-on: https://chromium-review.googlesource.com/c/1351323
Commit-Queue: David Staessens <dstaessens@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615024}
[modify] https://crrev.com/899dda5a9de3455c6d85e25c97e06b5fdf70d6f1/media/gpu/test/video_player/video.cc
[modify] https://crrev.com/899dda5a9de3455c6d85e25c97e06b5fdf70d6f1/media/gpu/test/video_player/video.h
[modify] https://crrev.com/899dda5a9de3455c6d85e25c97e06b5fdf70d6f1/media/gpu/test/video_player/video_decoder_client.cc
[modify] https://crrev.com/899dda5a9de3455c6d85e25c97e06b5fdf70d6f1/media/gpu/test/video_player/video_decoder_client.h
[modify] https://crrev.com/899dda5a9de3455c6d85e25c97e06b5fdf70d6f1/media/gpu/test/video_player/video_player.cc
[modify] https://crrev.com/899dda5a9de3455c6d85e25c97e06b5fdf70d6f1/media/gpu/test/video_player/video_player.h
[modify] https://crrev.com/899dda5a9de3455c6d85e25c97e06b5fdf70d6f1/media/gpu/video_decode_accelerator_tests.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 10

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

commit 045f9eb406eebb30948a205c760dd567221f1a82
Author: David Staessens <dstaessens@chromium.org>
Date: Mon Dec 10 02:09:31 2018

media/gpu/test: Add Reset tests to the new video decode test framework.

This CL adds support for the reset command to the test video player, and ports
some of the current VDA reset testscases to the new framework.

TEST=ran new VD tests on nocturne

BUG= 905952 ,905953

Change-Id: Ib82ac07b952823b9ddf6644ccc0268a507b37997
Reviewed-on: https://chromium-review.googlesource.com/c/1358104
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: David Staessens <dstaessens@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615030}
[modify] https://crrev.com/045f9eb406eebb30948a205c760dd567221f1a82/media/gpu/test/video_player/video_decoder_client.cc
[modify] https://crrev.com/045f9eb406eebb30948a205c760dd567221f1a82/media/gpu/test/video_player/video_decoder_client.h
[modify] https://crrev.com/045f9eb406eebb30948a205c760dd567221f1a82/media/gpu/test/video_player/video_player.cc
[modify] https://crrev.com/045f9eb406eebb30948a205c760dd567221f1a82/media/gpu/test/video_player/video_player.h
[modify] https://crrev.com/045f9eb406eebb30948a205c760dd567221f1a82/media/gpu/video_decode_accelerator_tests.cc

Status: Verified (was: Started)

Sign in to add a comment