When a change of stream occurs in a plain MP4, drain the decoder rather than flushing it

VERIFIED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
VERIFIED FIXED
6 months ago
4 months ago

People

(Reporter: jya, Assigned: jya)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55+ verified, firefox56 verified)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

6 months ago
When in a plain MP4 we detect a change of stream, we simply flush the decoder before re-creating a new one.

This can cause lots of frames to be lost as the windows decoder typically buffers over 30 frames at any given time.

It could be argued that those files are invalid, as no change of stream should ever occurred.
But we could make things nicer by draining the decoder first in order to retrieve all pending frames before recreating the decoder.
(Assignee)

Updated

6 months ago
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

6 months ago
mozreview-review
Comment on attachment 8878917 [details]
Bug 1374068: P1. Drain the decoder when content is changing.

https://reviewboard.mozilla.org/r/150156/#review154864

::: dom/media/platforms/wrappers/H264Converter.cpp:146
(Diff revision 1)
>    MOZ_RELEASE_ASSERT(mFlushPromise.IsEmpty(), "Previous flush didn't complete");
>  
>    /*
> -      When we detect a change of content in the H264 stream, we first flush the
> -    current decoder (1), shut it down (2) create a new decoder and initialize
> -    it (3).
> +    When we detect a change of content in the H264 stream, we first drain the
> +    current decoder (1), flush (2), shut it down (3) create a new decoder and
> +    initialize it (4). It is possible possible for H264Converter::Flush to be

Remove 2nd "possible".
(I see it was there before, but may as well fix typos while touching this comment.)

::: dom/media/platforms/wrappers/H264Converter.cpp:147
(Diff revision 1)
>  
>    /*
> -      When we detect a change of content in the H264 stream, we first flush the
> -    current decoder (1), shut it down (2) create a new decoder and initialize
> -    it (3).
> -    It is possible possible for H264Converter::Flush to be called during any of
> +    When we detect a change of content in the H264 stream, we first drain the
> +    current decoder (1), flush (2), shut it down (3) create a new decoder and
> +    initialize it (4). It is possible possible for H264Converter::Flush to be
> +    called during any of those time.

"time" -> "times"
Attachment #8878917 - Flags: review?(gsquelart) → review+

Comment 4

6 months ago
mozreview-review
Comment on attachment 8878918 [details]
Bug 1374068: P2. Rewrite bits with lambdas.

https://reviewboard.mozilla.org/r/150158/#review154866
Attachment #8878918 - Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

6 months ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f312f55453f
P1. Drain the decoder when content is changing. r=gerald
https://hg.mozilla.org/integration/autoland/rev/fd18e49efaaa
P2. Rewrite bits with lambdas. r=gerald
(Assignee)

Updated

6 months ago
Depends on: 1374210

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7f312f55453f
https://hg.mozilla.org/mozilla-central/rev/fd18e49efaaa
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 9

6 months ago
[Tracking Requested - why for this release]:

Bug 1372766 shows up on FF55 beta.  Can we get this uplifted there since this fixes that problem?
status-firefox55: --- → affected
tracking-firefox55: --- → ?
Looks like this would need bug 1374210 for uplift as well?
tracking-firefox55: ? → +
Flags: needinfo?(jyavenard)
(Assignee)

Comment 11

6 months ago
Yes, we will have to uplift those to 55.

However, not yet. There's something I want to explore first, so that we don't enter that code path in the first place.

All twitter videos are encoded in such a way that we continuously recreate a decoder, it's highly inefficient, and now I've found that it's the same with Dailymotion videos.
Flags: needinfo?(jyavenard)
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 12

6 months ago
Comment on attachment 8878917 [details]
Bug 1374068: P1. Drain the decoder when content is changing.

Request is for both patches.

Approval Request Comment
[Feature/Bug causing the regression]: 1313398
[User impact if declined]: Very janky playback of some files
[Is this code covered by automated tests?]: not directly.
[Has the fix been verified in Nightly?]: yes.
[Needs manual test from QE? If yes, steps to reproduce]: yes, see bug 1372766
[List of other uplifts needed for the feature/fix]: those are needed after uplifting this one: 1374210
[Is the change risky?]: there may be risks, but less so than not uplifting.
[Why is the change risky/not risky?]: I'm actively monitoring the issue, and currently still working on making it even less risky (preventing on entering this code path due to false positive) bug 1374774
[String changes made/needed]: None
Attachment #8878917 - Flags: approval-mozilla-beta?
Comment on attachment 8878917 [details]
Bug 1374068: P1. Drain the decoder when content is changing.

Maybe a bit risky, let's try this for beta 5 since it should fix some issues with Twitter video playback. We need to also uplift the patch in bug 1374210.
Attachment #8878917 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 14

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/44738df3b1b3
https://hg.mozilla.org/releases/mozilla-beta/rev/a55f95667183
status-firefox55: affected → fixed
Setting as qe-verify+. See bug 1372766 for steps to reproduce.
Flags: qe-verify+
I have managed to reproduce this issue by following the steps provided in bug 1372766 comment 0 using Firefox 56.0a1 (BuildId:20170613030203).

This issue is verified fixed on Firefox 55.0b12 (BuildId:20170724055319) using Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.11.6.

This issue is also verified fixed on Firefox Nightly 56.0a1 (BuildId:20170724030204) using Windows 10 64 bit, Ubuntu 16.04 64 bit and macOS 10.11.6.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: fixed → verified
Flags: qe-verify+
Blocks: 1372766
You need to log in before you can comment on or make changes to this bug.