1123498 - Make MP4Reader skip-to-next-keyframe less aggressively

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Sometimes skip-to-next-keyframe engages when we don't need it to; when if we just kept decoding a bit more, we'd be able to play all or at least more frames.

We tend to engage skip-to-next-keyframe more eagerly when switching streams, or exiting buffering. I'm not referring to what to do when the CPU/decoder is too slow to decode a video... There's not a lot we can do to win in that situation.

Comment 1

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

Attachment: Patch 1: Only drop GOPs that we can't play

For fragmented files (only) Have MP4Reader ignore the aSkipToNextKeyframe flag and instead only skip if the current playback position is behind the next keyframe. This means we'll only drop frames that we can't actually play.

Since this requires knowledge of where the next keyframe is, it's not generalizable to other Readers, like the WebM and Ogg Readers, until their demuxers are changed to allow that to be exported. So this change only has affect for the MP4Reader while using the MoofParser.

Comment 2

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

Attachment: Patch 2: test

A gtest. We can't land this until bug 1118593 is fixed, as MP4Demuxer::GetNextKeyframeTime() only works if we're using the MoofParser's SampleIterator, and we don't do that for CENC encrypted content since that fails on some files.

Comment 3

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

Landed patch 1 only:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9009dd32153

Will follow up with the gtest patch once edwin lands bug 1118597 (I meant bug 1118597 instead of bug 1118593 in comment 2).

ni? cpearce so I don't forget to land the test.
ni? rillian so we don't forget to uplift.

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 8551533 [details] [diff] [review]
Patch 1: Only drop GOPs that we can't play

Requesting approval for this patch only.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Video playback stalls on slower systems.
[Describe test coverage new/current, TBPL]: green on inbound.
[Risks and why]: This affects non-MSE mp4 playback. The main risk is that the new heuristic will play a sequence of still frames when it would have previously played smooth video.
[String/UUID change made/needed]: None.

Comment 5

[Carsten Book [:Tomcat]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9db42b98c37
https://hg.mozilla.org/releases/mozilla-beta/rev/cee6bfbbecd7

Comment 7

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

(In reply to Chris Pearce (:cpearce) from comment #3)
> Will follow up with the gtest patch once edwin lands bug 1118597 (I meant
> bug 1118597 instead of bug 1118593 in comment 2).
> 
> ni? cpearce so I don't forget to land the test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=359dd6cb8407

Comment 8

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

Landed gtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ff26901dd1

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10ff26901dd1

Comment 10

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d9e4d1fe90e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ralph Giles (:rillian) from comment #10)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/1d9e4d1fe90e

Backed out for checktest failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/751da1d2f7b1

https://treeherder.mozilla.org/logviewer.html#?job_id=559652&repo=mozilla-aurora