1259366 - Buffering does not end during youtube video playback on b2g aries

Status: RESOLVED FIXED

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

Description

[Sotaro Ikeda [:sotaro]]

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.

Comment 1

[Sotaro Ikeda [:sotaro]]

When the problem happened, VideoQueue in VideoSink has 4 video buffers. It seems to have enough video buffers to resume video playback.

Comment 2

[Sotaro Ikeda [:sotaro]]

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.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

I fixed a buffering bug (bug 1258627) the other day. Can you check if the patch fixes your problem?

Comment 4

[Sotaro Ikeda [:sotaro]]

(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.

Comment 5

[Sotaro Ikeda [:sotaro]]

The problem was caused by audio decoding. GonkAudioDecoderManager::Output() received -EAGAIN from android::MediaDecoder and audio decoding was stopped and AudioQueue became empty.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Flush after eos of MediaDecoder

android::MediaDecoder needs to be flushed after eos to receive next input.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Keep ref count of MediaData until MediaFormatReader::NotifyNewOutput()

MediaData is a ref counted object. It is necessary to keep ref count to keep the object alive.

Comment 8

[Jean-Yves Avenard [:jya]]

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.

Comment 9

[Sotaro Ikeda [:sotaro]]

(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 10

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8734753 [details] [diff] [review]
patch part 2 - Keep ref count of MediaData until MediaFormatReader::NotifyNewOutput()

Yea. It is not necessary.

Comment 11

[Jean-Yves Avenard [:jya]]

(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.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Flush after eos of MediaDecoder

Fix as mAudioQueue.Reset() called.

Comment 13

[John Lin [:jhlin][: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.

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

I going to update by a safer patch.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Flush after eos of MediaDecoder

Comment 17

[Sotaro Ikeda [:sotaro]]

(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!

Comment 18

[Jean-Yves Avenard [:jya]]

(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 19

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

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.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Flush after eos of MediaDecoder

Apply the comment. Carry "r=jolin".

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Flush after eos of MediaDecoder

Correct patch.

Comment 22

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

 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

Comment 23

[Sotaro Ikeda [:sotaro]]

:jolin, how do we change GonkMediaDataDecoder::Drain()? Similar to flush?

Comment 24

[Sotaro Ikeda [:sotaro]]

(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?

Comment 25

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

(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.

Comment 26

[Sotaro Ikeda [:sotaro]]

Okey, thanks.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a043400e5be

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a043400e5be