Closed Bug 1345599 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Platform:
All
Android
Type:
defect

Tracking

(fennec54+, firefox52 disabled, firefox53+ disabled, firefox54 fixed, firefox55 verified)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
fennec 54+ ---
firefox52 --- disabled
firefox53 + disabled
firefox54 --- fixed
firefox55 --- verified

People

(Reporter: JanH, Assigned: jhlin)

References

(
URL
)

Details

Attachments

(3 files)

Bug 1345599 - part 1: reduce InputExhausted() calls to avoid input queue flood.
59 bytes, text/x-review-board-request
snorp
: review+
gchang
: approval-mozilla-aurora+
Details
Bug 1345599 - part 2: queue empty buffer when fail to copy input data.
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1345599 - part 3: for recyclable decoder, don't drain/flush when stream ID/SPS change.
59 bytes, text/x-review-board-request
jya
: review+
Details 
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.
Flags: needinfo?(jolin)
Sounds like maybe the decoder is crashing?
Assignee: nobody → jolin
tracking-fennec: ? → 54+
There seems serious leakage of shared memory used by OOP decoding IPC. Investigating now.
Flags: needinfo?(jolin)
Priority: -- → P1
[Tracking Requested - why for this release]:

Since OOP is enabled on 53, we should track it on 53 as well.
tracking-firefox53: --- → ?
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 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
Attachment #8847029 - Flags: review?(jyavenard) → review+
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 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 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 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 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.
Attachment #8847027 - Flags: review?(snorp) → review+
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
Attachment #8847028 - Flags: review?(snorp) → review+
Track 53+ as youtube playback is important to uesrs and this is user-visible issue.
tracking-firefox53: ? → +
Pushed by jolin@mozilla.com:
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
Working fine now, thanks for fixing.
status-firefox55: fixed → verified
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
Attachment #8847027 - Flags: approval-mozilla-aurora?
Blocks: 1345342
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+.
Attachment #8847027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
status-firefox52: ? → disabled
status-firefox53: ? → disabled
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: