Closed Bug 1107274 Opened 5 years ago Closed 5 years ago

video playback skip last few frames

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- unaffected
firefox36 + fixed
firefox37 + fixed

People

(Reporter: jya, Assigned: mattwoodrow)

References

Details

(Keywords: regression)

Attachments

(1 file)

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)
148 frames with VDA as well.

It looks like MP4Reader is never calling ::Drain() at the end of the video.
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.
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.
Blocks: 1032633
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.
Assignee: nobody → matt.woodrow
Attachment #8531768 - Flags: review?(cpearce)
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
Attachment #8531768 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/c8c840206206
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Matt, can you own getting a test checked in for this, or suggest someone?
Flags: needinfo?(matt.woodrow)
I'm not sure how to create an appropriate test video for this.

Chris, do you have any ideas?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(cpearce)
Flags: in-testsuite?
Can't we just capture the last frame, and do a reftest that plays it and compares the last image to the captured frame?
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.
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.
Flags: needinfo?(cpearce)
(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.
Duplicate of this bug: 1106763
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?
Flags: needinfo?(ajones)
Flags: needinfo?(spolk)
I will be working on web-platform-tests first. gtest will come later.
Flags: needinfo?(spolk)
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
Attachment #8531768 - Flags: approval-mozilla-aurora?
Attachment #8531768 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Filed a separate bug for the last frame test
Flags: needinfo?(ajones)
Carrying over status and tracking flags from dup bug 1106763.
You need to log in before you can comment on or make changes to this bug.