Closed Bug 1259366 Opened 8 years ago Closed 8 years ago

Buffering does not end during youtube video playback on b2g aries

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 5 obsolete files)

When a patch of Bug 1252835 is applied, sometimes buffering does not end during youtube video playback on b2g aries.

From media framework point of view, the patch just change the timing of TextureClient recycle callback. The problem happened even when TextureHost side's recycling was disabled.
When the problem happened, VideoQueue in VideoSink has 4 video buffers. It seems to have enough video buffers to resume video playback.
Blocks: 1252835
I tested with the following video
 https://www.youtube.com/watch?v=e-ORhEE9VVg

STR
[1] start the video playback.
[2] seek video and confirm buffering ui is shown.

After [2], if video playback resume normally continue until the problem happens.
Assignee: nobody → sotaro.ikeda.g
I fixed a buffering bug (bug 1258627) the other day. Can you check if the patch fixes your problem?
(In reply to JW Wang [:jwwang] from comment #3)
> I fixed a buffering bug (bug 1258627) the other day. Can you check if the
> patch fixes your problem?

Thanks! My code did not have bug 1258627 fix. But the problem did not addressed by applying the patch.
The problem was caused by audio decoding. GonkAudioDecoderManager::Output() received -EAGAIN from android::MediaDecoder and audio decoding was stopped and AudioQueue became empty.
android::MediaDecoder needs to be flushed after eos to receive next input.
MediaData is a ref counted object. It is necessary to keep ref count to keep the object alive.
Attachment #8734751 - Flags: review?(jolin)
Attachment #8734753 - Flags: review?(jyavenard)
Comment on attachment 8734753 [details] [diff] [review]
patch part 2 - Keep ref count of MediaData until MediaFormatReader::NotifyNewOutput()

Review of attachment 8734753 [details] [diff] [review]:
-----------------------------------------------------------------

Because MediaData has a AddRef method, it is automatically addrefed.

See bug 1153295.
Attachment #8734753 - Flags: review?(jyavenard) → review-
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> Created attachment 8734751 [details] [diff] [review]
> patch part 1 - Flush after eos of MediaDecoder
> 
> android::MediaDecoder needs to be flushed after eos to receive next input.

It might cause a problem like Bug 1199809. But there seems no other way to address the eos problem. It seems better to enable OmxDecoderModule soon.
Comment on attachment 8734753 [details] [diff] [review]
patch part 2 - Keep ref count of MediaData until MediaFormatReader::NotifyNewOutput()

Yea. It is not necessary.
Attachment #8734753 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> Comment on attachment 8734753 [details] [diff] [review]
> patch part 2 - Keep ref count of MediaData until
> MediaFormatReader::NotifyNewOutput()
> 
> Yea. It is not necessary.

you would have had massive amount of crashes if the MediaData wasn't kept alive long enough as it typically would have been deleted immediately upon returning from MediaFormatReader::Output

BTW, if there wasn't an automatic AddRef, you wouldn't have been able to use a RefPtr<MediaData> instead you would have had to use StorensRefPtrPassByPtr. The RefPtr passed would have gone out of scope too. You would have to manually call AddRef and extra time.
Attachment #8734751 - Flags: review?(jolin)
Fix as mAudioQueue.Reset() called.
Attachment #8734751 - Attachment is obsolete: true
Attachment #8735058 - Flags: review?(jolin)
(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> Created attachment 8735058 [details] [diff] [review]
> patch - Flush after eos of MediaDecoder
> 
> Fix as mAudioQueue.Reset() called.
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > Created attachment 8734751 [details] [diff] [review]
> > patch part 1 - Flush after eos of MediaDecoder
> > 
> > android::MediaDecoder needs to be flushed after eos to receive next input.
> 
> It might cause a problem like Bug 1199809. But there seems no other way to
> address the eos problem. It seems better to enable OmxDecoderModule soon.

I'm not sure what the eos problem is here. Could you please give more details about it? Thanks a lot.
Flags: needinfo?(sotaro.ikeda.g)
ACodec does not receive input buffer if input port is EOS state like the following. Majority of cases, the EOS flag is unset by flush. But there is a case that flush is not called by MediaFormatReader when the code is in eos state.

>void ACodec::BaseState::postFillThisBuffer(BufferInfo *info) {
>    if (mCodec->mPortEOS[kPortIndexInput]) {
>        return;
>    }

 > http://androidxref.com/6.0.1_r10/xref/frameworks/av/media/libstagefright/ACodec.cpp#4812

This seems same to Open MAX IL component. The following is from OpenMAX IL spec.

> 3.1.2.6.13.1 OMX_BUFFERFLAG_EOS
> A component sets EOS when it has no more data to emit on a particular output port. Thus,
> an output port shall set EOS on the last buffer it emits. The determination by a
> component of when an output port should cease sending data is implementation specific.

OmxDataDecoder::EndOfStream() already handle the case.
  > https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/omx/OmxDataDecoder.cpp#135
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8735058 - Flags: review?(jolin)
I going to update by a safer patch.
Attachment #8735058 - Attachment is obsolete: true
Attachment #8735662 - Flags: review?(jolin)
(In reply to Jean-Yves Avenard [:jya] from comment #11)
> 
> BTW, if there wasn't an automatic AddRef, you wouldn't have been able to use
> a RefPtr<MediaData> instead you would have had to use
> StorensRefPtrPassByPtr. The RefPtr passed would have gone out of scope too.
> You would have to manually call AddRef and extra time.

Thanks for the advice!
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> ACodec does not receive input buffer if input port is EOS state like the
> following. Majority of cases, the EOS flag is unset by flush. But there is a
> case that flush is not called by MediaFormatReader when the code is in eos
> state.

Flush() is only called if we're about to seek (or prior the decoder being fed discontinuous data).

Flush() isn't called when reaching EOS, only Drain() is.

The reason you're seeing Flush being called can only be occurring with MSE when we reach a discontinuity (gap in the buffered range). We drain the decoder to ensure that all decoded frames are output, and it will then seek back to the discontinuity.
If the mediasource is ended or it's a real EOS, then no internal seek occurs, and as such Flush() isn't called.
Comment on attachment 8735662 [details] [diff] [review]
patch - Flush after eos of MediaDecoder

Review of attachment 8735662 [details] [diff] [review]:
-----------------------------------------------------------------

This makes sense to me now. Thanks a lot.

::: dom/media/platforms/gonk/GonkMediaDataDecoder.cpp
@@ +247,5 @@
>        }
>        MOZ_ASSERT(mWaitOutput.Length() == 1);
>        mWaitOutput.RemoveElementAt(0);
>        mDecodeCallback->DrainComplete();
> +      // After eos, android::MediaCodec needs to be flushed to receive next input

Suggest to move this comment to the implementation of RestEOS() to make why calling flush() there more clear.
Attachment #8735662 - Flags: review?(jolin) → review+
Apply the comment. Carry "r=jolin".
Attachment #8735662 - Attachment is obsolete: true
Attachment #8735678 - Flags: review+
Correct patch.
Attachment #8735678 - Attachment is obsolete: true
Attachment #8735682 - Flags: review+
 In current implementation, GonkDeocoderManager assumes Drain() is only called when EOS, but it seems there are other cases? [1][2]
 If that assumption is not valid, we should also consider modify the implementation of GonkMediaDataDecoder::Drain().

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp?from=MediaFormatReader.cpp#553
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp?from=MediaFormatReader.cpp#904
:jolin, how do we change GonkMediaDataDecoder::Drain()? Similar to flush?
Flags: needinfo?(jolin)
(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> :jolin, how do we change GonkMediaDataDecoder::Drain()? Similar to flush?

If we need to update the function, can you address it in another bug?
(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> (In reply to Sotaro Ikeda [:sotaro] from comment #23)
> > :jolin, how do we change GonkMediaDataDecoder::Drain()? Similar to flush?
> 
> If we need to update the function, can you address it in another bug?

 I think your patch fixes the problem just fine. Please land it. Thanks.
Flags: needinfo?(jolin)
Okey, thanks.
https://hg.mozilla.org/mozilla-central/rev/7a043400e5be
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.