Make MP4Reader skip-to-next-keyframe less aggressively

RESOLVED FIXED in Firefox 36



4 years ago
4 years ago


(Reporter: cpearce, Assigned: cpearce)


(Blocks 1 bug)

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)



(2 attachments)

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

4 years ago
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.
Attachment #8551533 - Flags: review?(matt.woodrow)

Comment 2

4 years ago
Posted patch Patch 2: testSplinter Review
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.
Attachment #8551534 - Flags: review?(matt.woodrow)
Attachment #8551533 - Flags: review?(matt.woodrow) → review+
Attachment #8551534 - Flags: review?(matt.woodrow) → review+

Comment 3

4 years ago
Landed patch 1 only:

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.
Depends on: 1118597
Flags: needinfo?(giles)
Flags: needinfo?(cpearce)
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.
Flags: needinfo?(giles)
Attachment #8551533 - Flags: approval-mozilla-beta?
Attachment #8551533 - Flags: approval-mozilla-aurora?
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8551533 - Flags: approval-mozilla-beta?
Attachment #8551533 - Flags: approval-mozilla-beta+
Attachment #8551533 - Flags: approval-mozilla-aurora?
Attachment #8551533 - Flags: approval-mozilla-aurora+

Comment 7

4 years ago
(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.


4 years ago
Flags: in-testsuite? → in-testsuite+
See Also: → 1131563
You need to log in before you can comment on or make changes to this bug.