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.
Created attachment 8551533 [details] [diff] [review] 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.
Attachment #8551533 - Flags: review?(matt.woodrow)
Created attachment 8551534 [details] [diff] [review] 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.
Attachment #8551534 - Flags: review?(matt.woodrow)
Attachment #8551533 - Flags: review?(matt.woodrow) → review+
Attachment #8551534 - Flags: review?(matt.woodrow) → review+
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.
Depends on: 1118597
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.
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
status-firefox36: affected → fixed
status-firefox37: affected → fixed
status-firefox38: affected → fixed
(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
Flags: in-testsuite? → in-testsuite+
(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
You need to log in before you can comment on or make changes to this bug.