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

Status: VERIFIED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1374068: P1. Drain the decoder when content is changing.

Review commit: https://reviewboard.mozilla.org/r/150156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150156/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1374068: P2. Rewrite bits with lambdas.

Allow to get rid of the mPendingSample member, making the logic easier to follow.

Review commit: https://reviewboard.mozilla.org/r/150158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150158/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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"

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8878918 [details]
Bug 1374068: P2. Rewrite bits with lambdas.

https://reviewboard.mozilla.org/r/150158/#review154866

Comment 5

[Jean-Yves Avenard [:jya]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150156/diff/1-2/

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8878918 [details]
Bug 1374068: P2. Rewrite bits with lambdas.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150158/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f312f55453f
https://hg.mozilla.org/mozilla-central/rev/fd18e49efaaa

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

[Tracking Requested - why for this release]:

Bug 1372766 shows up on FF55 beta.  Can we get this uplifted there since this fixes that problem?

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like this would need bug 1374210 for uplift as well?

Comment 11

[Jean-Yves Avenard [:jya]]

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.

Comment 12

[Jean-Yves Avenard [:jya]]

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

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/44738df3b1b3
https://hg.mozilla.org/releases/mozilla-beta/rev/a55f95667183

Comment 15

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting as qe-verify+. See bug 1372766 for steps to reproduce.

Comment 16

[Emil Ghitta, QA [:emilghitta]]

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.