Closed Bug 1374068 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + verified
firefox56 --- verified

People

(Reporter: jya, Assigned: jya)

References

()

Details

Attachments

(2 files)

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.
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 on attachment 8878918 [details]
Bug 1374068: P2. Rewrite bits with lambdas.

https://reviewboard.mozilla.org/r/150158/#review154866
Attachment #8878918 - Flags: review?(gsquelart) → review+
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
Depends on: 1374210
https://hg.mozilla.org/mozilla-central/rev/7f312f55453f
https://hg.mozilla.org/mozilla-central/rev/fd18e49efaaa
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
[Tracking Requested - why for this release]:

Bug 1372766 shows up on FF55 beta.  Can we get this uplifted there since this fixes that problem?
Looks like this would need bug 1374210 for uplift as well?
Flags: needinfo?(jyavenard)
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)
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+
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.