1107274 - video playback skip last few frames

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

Example:
http://people.mozilla.org/~ajones/mse/test.mp4

video is exactly 6 seconds and 150 frames long. Only 146 frames are displayed when using FFmpeg (or 148 frames when using VideoToolbox)...

This is a regression, used to work fine...

(suspecting the asynchronous decode change)

Comment 1

[Ralph Giles (:rillian)]

148 frames with VDA as well.

It looks like MP4Reader is never calling ::Drain() at the end of the video.

Comment 2

[Ralph Giles (:rillian)]

Correction. I was instrumenting the VDA path but it was running the VT path. We do call drain to flush the two queued frames at the end.

Comment 3

[Ralph Giles (:rillian)]

Confirmed the regression is the result of the async decode patch in bug 1032633.

Analysis from Matt Woodrow: MP4Reader calls Drain(), then closes everything down after OnDrainCompleted() even though there are still frames in its queue. Instead it needs to maintain separate EOS flags for the demuxer and the decoder and wait for MediaDecoderStateMachine to finish pulling all the frames.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Make sure we return all frames before returning EOS

We really need to add a test for this.

If we can get a video with only two frames (both keyframes) that are different colours, then we should be able to make a reftest for this easily.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8531768 [details] [diff] [review]
Make sure we return all frames before returning EOS

Review of attachment 8531768 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/fmp4/MP4Reader.cpp
@@ +771,5 @@
>    // MediaDataDecoder::Flush().
>    {
>      MonitorAutoLock mon(data.mMonitor);
>      data.mIsFlushing = true;
> +    data.mDemuxEOS = false;

I think you need to reset mDecodeEOS here too.

::: dom/media/fmp4/MP4Reader.h
@@ +188,5 @@
>      bool mIsFlushing;
>      bool mOutputRequested;
>      bool mUpdateScheduled;
> +    bool mDemuxEOS;
> +    bool mDecodeEOS;

I would prefer mDrainComplete over mDecodeEOS

Comment 6

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c840206206

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8c840206206

Comment 8

[Bobby Holley (:bholley)]

Matt, can you own getting a test checked in for this, or suggest someone?

Comment 9

[Matt Woodrow (:mattwoodrow)]

I'm not sure how to create an appropriate test video for this.

Chris, do you have any ideas?

Comment 10

[Bobby Holley (:bholley)]

Can't we just capture the last frame, and do a reftest that plays it and compares the last image to the captured frame?

Comment 11

[Matt Woodrow (:mattwoodrow)]

The last frame would need to be a keyframe, otherwise we might not actually show if it we hit the skip to keyframe logic in MDSM.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem with writing a reftest is that we have frame-dropping logic that could kick in and we'll end up dropping the target frame. This especially is a problem on our test machines when they're under load, and we'd get intermittent failures.

We could write a gtest to manually run the MP4Reader. Or we could add a pref to disable *all* frame dropping/skip-to-keyframe for testing purposes, and then a reftest may work.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Chris Pearce (:cpearce) from comment #12)
> We could write a gtest to manually run the MP4Reader. Or we could add a pref
> to disable *all* frame dropping/skip-to-keyframe for testing purposes, and
> then a reftest may work.

IMO we should do this. The ability to reliably test video end states seems pretty critical for good test coverage.

I don't want to create unnecessary distraction. But I'm also unhappy with the current state of affairs in which review comments often include "can you manually check the testcases from bug X and bug Y to make sure this doesn't regress them?" - this process does not scale.

Comment 15

[Bobby Holley (:bholley)]

Anthony - comment 13 seems like a great thing to get QA's help on. I may or may not make it to the QA coordination meeting on Wednesday - if I don't, can you make sure to bring this up?

Comment 16

[Syd Polk :sydpolk]

I will be working on web-platform-tests first. gtest will come later.

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8531768 [details] [diff] [review]
Make sure we return all frames before returning EOS

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent playback support, video sites more
likely to use flash.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Low; bug fix to playback handling.
[String/UUID change made/needed]: None

Comment 18

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/683b0185016a

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Filed a separate bug for the last frame test

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Carrying over status and tracking flags from dup bug 1106763.