1345599 - Youtube playback is consistently interrupted and restarts (sometimes with worse quality)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Description

[Jan Henning [:JanH]]

Before bug 1340160, the linked video (https://www.youtube.com/watch?v=qzVGqJH3-ww) would consistently crash around the minute mark. No it no longer crashes, but playback is still momentarily interrupted. Additionally, on some occasions the video quality after the restart is noticeably worse.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Sounds like maybe the decoder is crashing?

Comment 2

[John Lin [:jhlin][:jolin]]

There seems serious leakage of shared memory used by OOP decoding IPC. Investigating now.

Comment 3

[Blake Wu [:bwu][:blakewu]]

[Tracking Requested - why for this release]:

Since OOP is enabled on 53, we should track it on 53 as well.

Comment 4

[John Lin [:jhlin][:jolin]]

Turns out it's not leak.
After bug 1319987 Java callbacks onOutput() and onInputExhausted() both resolve DecodePromise. This will make format reader feeds input samples much faster than decoder can consume them and tens of video input samples could be queued in Codec.InputProcessor.mInputSamples. Will find some way to throttle to avoid the input flood.

Comment 5

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1345599 - part 1: reduce InputExhausted() calls to avoid input queue flood.

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

Comment 6

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1345599 - part 2: queue empty buffer when fail to copy input data.

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

Comment 7

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1345599 - part 3: for recyclable decoder, don't drain/flush when stream ID/SPS change.

Stream ID change doesn't always come with SPS change and after draining Android decoder won't accept more input without flushing.
Plus, flushing after draining will invalid all output buffers collected by draining and causes missing frames.

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

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8847029 [details]
Bug 1345599 - part 3: for recyclable decoder, don't drain/flush when stream ID/SPS change.

https://reviewboard.mozilla.org/r/120070/#review121886

::: commit-message-ca055:5
(Diff revision 1)
> +Bug 1345599 - part 3: for recyclable decoder, don't drain/flush when stream ID/SPS change. r?jya
> +
> +Stream ID change doesn't always come with SPS change and after draining Android decoder won't accept more input without flushing.
> +Plus, flushing after draining will invalid all output buffers collected by draining and causes missing frames.
> +

that comment is incorrect. A stream ID change always come after a new init segment has been encountered.

As such, there will always be a change of SPS/PPS when the stream ID change.

Plus this change makes no sense.
The title states to not drain/flush but the next line says that you need to flush after drain.

Please correct.

::: dom/media/MediaFormatReader.cpp:1963
(Diff revision 1)
>    while (decoder.mQueuedSamples.Length()) {
>      RefPtr<MediaRawData> sample = decoder.mQueuedSamples[0];
>      RefPtr<TrackInfoSharedPtr> info = sample->mTrackInfo;
>  
>      if (info && decoder.mLastStreamSourceID != info->GetID()) {
> -      if (decoder.mNextStreamSourceID.isNothing()
> +      bool unrecyclable = !(MediaPrefs::MediaDecoderCheckRecycling()

I would much prefer that the name be "recyclable" and you use !recyclable.

::: dom/media/MediaFormatReader.cpp:1967
(Diff revision 1)
>      if (info && decoder.mLastStreamSourceID != info->GetID()) {
> -      if (decoder.mNextStreamSourceID.isNothing()
> -          || decoder.mNextStreamSourceID.ref() != info->GetID()) {
> +      bool unrecyclable = !(MediaPrefs::MediaDecoderCheckRecycling()
> +                            && decoder.mDecoder->SupportDecoderRecycling());
> +      if (unrecyclable
> +          && (decoder.mNextStreamSourceID.isNothing()
> +             || decoder.mNextStreamSourceID.ref() != info->GetID())) {

incorrect placement of ||.
Either align it with decoder above, or place it at the end of the previous line

::: dom/media/platforms/wrappers/H264Converter.cpp:335
(Diff revision 1)
>    RefPtr<MediaRawData> sample = aSample;
>  
>    if (CanRecycleDecoder()) {
>      // Do not recreate the decoder, reuse it.
>      UpdateConfigFromExtraData(extra_data);
>      // Ideally we would want to drain the decoder instead of flushing it.

that comment is no longer valid either. please remove entirely

::: dom/media/platforms/wrappers/H264Converter.cpp:346
(Diff revision 1)
> -               mFlushRequest.Complete();
> -               mDecodePromise.Reject(aError, __func__);
> -             })
> -      ->Track(mFlushRequest);
>      mNeedKeyframe = true;
>      // This is not really initializing the decoder, but it will do as it

you need to remove this comment, it's no longer valid

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8847027 [details]
Bug 1345599 - part 1: reduce InputExhausted() calls to avoid input queue flood.

https://reviewboard.mozilla.org/r/120066/#review121888

::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:138
(Diff revision 1)
>                      mCodec.queueSecureInputBuffer(index, 0, cryptoInfo, pts, flags);
>                  } else {
>                      mCodec.queueInputBuffer(index, 0, len, pts, flags);
>                  }
>              }
> +            if (mDequeuedSamples.size() + mInputSamples.size() <= 2) {

where is this 2 coming from?

Is this in relation to the max_num_ref_frames SPS parameter, and if so it can vary...

Additionally, if you're not calling InputExhausted, the decoder won't be fed a new data after the first input, so how could mInputSamples size ever become greater than 2 then?

Comment 10

[John Lin [:jhlin][:jolin]]

Comment on attachment 8847027 [details]
Bug 1345599 - part 1: reduce InputExhausted() calls to avoid input queue flood.

https://reviewboard.mozilla.org/r/120066/#review121888

> where is this 2 coming from?
> 
> Is this in relation to the max_num_ref_frames SPS parameter, and if so it can vary...
> 
> Additionally, if you're not calling InputExhausted, the decoder won't be fed a new data after the first input, so how could mInputSamples size ever become greater than 2 then?

2 here is just arbitrary number saying there is still *some* buffers waiting to be processed in the queue. It is not related to max_num_of_ref_frames.
Once the decoder starts consuming these pending buffers, the size of queue decreases and `InputExhausted` will be called.

Comment 11

[John Lin [:jhlin][:jolin]]

Comment on attachment 8847029 [details]
Bug 1345599 - part 3: for recyclable decoder, don't drain/flush when stream ID/SPS change.

https://reviewboard.mozilla.org/r/120070/#review121886

> that comment is incorrect. A stream ID change always come after a new init segment has been encountered.
> 
> As such, there will always be a change of SPS/PPS when the stream ID change.
> 
> Plus this change makes no sense.
> The title states to not drain/flush but the next line says that you need to flush after drain.
> 
> Please correct.

Some live streams on YouTube sends init segment but `H264Converter` doesn't recognise SPS change. Perhaps the logic in `CheckSPSChange` is flawed? Currently it compares `extra_data` in MediaRawData, two *identical* extra_data could be considered non-change.

In the next line I intended to explain why drain/flush is harmful to Android decoder in this case:
- no flush after drain causes stop
- flush after drain causes missing frames
Looks like I did a bad job and made it more confusing. T_T
Will remove that line.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8847029 [details]
Bug 1345599 - part 3: for recyclable decoder, don't drain/flush when stream ID/SPS change.

https://reviewboard.mozilla.org/r/120070/#review121886

> Some live streams on YouTube sends init segment but `H264Converter` doesn't recognise SPS change. Perhaps the logic in `CheckSPSChange` is flawed? Currently it compares `extra_data` in MediaRawData, two *identical* extra_data could be considered non-change.
> 
> In the next line I intended to explain why drain/flush is harmful to Android decoder in this case:
> - no flush after drain causes stop
> - flush after drain causes missing frames
> Looks like I did a bad job and made it more confusing. T_T
> Will remove that line.

CheckSPSChange is only there to catter for plain MP4 that have a change of content in between (or AVC3 content).
You could actually fully get rid of it for MSE because the MFR has already done the job.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8847027 [details]
Bug 1345599 - part 1: reduce InputExhausted() calls to avoid input queue flood.

https://reviewboard.mozilla.org/r/120066/#review122632

looks good with one nit

::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:138
(Diff revision 1)
>                      mCodec.queueSecureInputBuffer(index, 0, cryptoInfo, pts, flags);
>                  } else {
>                      mCodec.queueInputBuffer(index, 0, len, pts, flags);
>                  }
>              }
> +            if (mDequeuedSamples.size() + mInputSamples.size() <= 2) {

I think you should put the '2' into a static variable with a comment indicating how it's used.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8847028 [details]
Bug 1345599 - part 2: queue empty buffer when fail to copy input data.

https://reviewboard.mozilla.org/r/120068/#review122636

Comment 16

[John Lin [:jhlin][:jolin]]

Comment on attachment 8847027 [details]
Bug 1345599 - part 1: reduce InputExhausted() calls to avoid input queue flood.

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

Comment 17

[John Lin [:jhlin][:jolin]]

Comment on attachment 8847028 [details]
Bug 1345599 - part 2: queue empty buffer when fail to copy input data.

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

Comment 18

[John Lin [:jhlin][:jolin]]

Comment on attachment 8847029 [details]
Bug 1345599 - part 3: for recyclable decoder, don't drain/flush when stream ID/SPS change.

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

Comment 19

[Gerry Chang [:gchang]]

Track 53+ as youtube playback is important to uesrs and this is user-visible issue.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a239e4b25e30
part 1: reduce InputExhausted() calls to avoid input queue flood. r=snorp
https://hg.mozilla.org/integration/autoland/rev/f88bcad96d78
part 2: queue empty buffer when fail to copy input data. r=snorp
https://hg.mozilla.org/integration/autoland/rev/be76983050c7
part 3: for recyclable decoder, don't drain/flush when stream ID/SPS change. r=jya

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a239e4b25e30
https://hg.mozilla.org/mozilla-central/rev/f88bcad96d78
https://hg.mozilla.org/mozilla-central/rev/be76983050c7

Comment 22

[Jan Henning [:JanH]]

Working fine now, thanks for fixing.

Comment 23

[John Lin [:jhlin][:jolin]]

Comment on attachment 8847027 [details]
Bug 1345599 - part 1: reduce InputExhausted() calls to avoid input queue flood.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1336431
[User impact if declined]: browser crash
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: it removes unnecessary operations. 
[String changes made/needed]: none

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8847027 [details]
Bug 1345599 - part 1: reduce InputExhausted() calls to avoid input queue flood.

Fix a video playback issue and was verified by reporter. Aurora54+.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/05835c70232e
https://hg.mozilla.org/releases/mozilla-aurora/rev/d2d9582aca65
https://hg.mozilla.org/releases/mozilla-aurora/rev/d2727be705bd

Comment 26

[Jan Henning [:JanH]]

I guess that bug 1350209 means that we're safe on Beta anyway now, no matter whether it is actually affected by this or not (and on 52 it's not enabled anyway).