1122447 - PlatformDeocdeModule fails to pass test_autoplay_contentEditable.html

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alfredo Yang (:alfredo)]

01-16 09:23:34.590 W/MPEG4Extractor(  690): Error -1004 parsing chuck at offset 0 looking for first track
01-16 09:23:34.630 W/MPEG4Extractor(  690): Error -1004 parsing chuck at offset 451626 looking for first track
01-16 09:23:34.700 I/OMXClient(  690): Using client-side OMX mux.
01-16 09:23:34.700 D/GonkAudioDecoderManager(  690): Init Audio channel no:1, sample-rate:48000
01-16 09:23:34.730 V/IMediaResourceManagerDeathNotifier(  690): getMediaResourceManagerService
01-16 09:23:34.730 I/OMXClient(  690): Using client-side OMX mux.
01-16 09:23:34.770 D/GonkVideoDecoderManager(  690): CodecReserved!
01-16 09:23:34.830 I/SoftAAC2(  690): Reconfiguring decoder: 0->48000 Hz, 0->1 channels
01-16 09:23:34.890 D/GonkAudioDecoderManager(  690): Decoder format changed
01-16 09:23:34.890 D/GonkAudioDecoderManager(  690): Decoder format changed
01-16 09:23:35.130 I/GeckoDump(  690): [SimpleTest.finish()] this test already called finish!
01-16 09:23:36.160 D/MediaCodecProxy(  690): dequeueInputBuffer returned -11
01-16 09:23:36.160 I/Gecko   (  690): [Child 690] WARNING: GonkAudioDecoder failed to input data: file ../../../../../dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp, line 77
01-16 09:23:36.160 D/GonkMediaDataDecoder(  690): Failed to input data err: -2147467259
01-16 09:23:36.160 I/Gecko   (  690): [Child 690] WARNING: Decoder=b1d982e0 Decode error, changed state to SHUTDOWN due to error: file ../../../dom/media/MediaDecoderStateMachine.cpp, line 2113

Comment 1

[Alfredo Yang (:alfredo)]

Root cause:
OMX dequeueInput buffer could fail at [1].

This bug also causes gonk MSE playback stopping when changing resolution.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp#75

Comment 2

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

How does it result in test_autoplay_contentEditable.html failure? Unable to decode metadata?

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: handle_input_error

Comment 4

[Alfredo Yang (:alfredo)]

(In reply to JW Wang [:jwwang] from comment #2)
> How does it result in test_autoplay_contentEditable.html failure? Unable to
> decode metadata?

OMX fails to dequeue input buffer and sends error back to MDSM in DECODING_FIRSTFRAME.

Comment 5

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

Comment on attachment 8552246 [details] [diff] [review]
handle_input_error

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

If the error is -ETIMEDOUT or -EAGAIN, 
Will this input encoded frame be dropped? If yes, should we queue the input encoded frame to try again later?

Comment 6

[Alfredo Yang (:alfredo)]

(In reply to Blake Wu [:bwu][:blakewu] from comment #5)
> Comment on attachment 8552246 [details] [diff] [review]
> handle_input_error
> 
> Review of attachment 8552246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If the error is -ETIMEDOUT or -EAGAIN, 
> Will this input encoded frame be dropped? If yes, should we queue the input
> encoded frame to try again later?

Yes, that's a problem. We need to resend these data, not discard them.

Comment 7

[Alfredo Yang (:alfredo)]

And we probably need to queue MP4Sample somewhere for retry later...

Comment 8

[Alfredo Yang (:alfredo)]

Attachment: handle_input_error

Comment 9

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

Comment on attachment 8552894 [details] [diff] [review]
handle_input_error

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

Overall it looks good to me! 
Just have some concerns on memory and flush case. 
For seek case, we should need to flush all the samples in mQueueSample.

::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp
@@ +29,5 @@
> +{
> +  nsAutoPtr<mp4_demuxer::MP4Sample> sample;
> +
> +  if (aSample) {
> +    sample = new mp4_demuxer::MP4Sample(*aSample);

New memory and memcpy will be done here. Is there any way to avoid this? Or could we only queue those samples that fail to be fed to decoder and need to try again?

@@ +32,5 @@
> +  if (aSample) {
> +    sample = new mp4_demuxer::MP4Sample(*aSample);
> +    PerformFormatSpecificProcess(sample);
> +  } else {
> +    sample = new mp4_demuxer::MP4Sample();

May need to have some comment to explain why we should new a sample if aSample = nullptr.

Comment 10

[Alfredo Yang (:alfredo)]

Attachment: handle_input_error

Comment 11

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

Comment on attachment 8553569 [details] [diff] [review]
handle_input_error

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

LGTM!

::: dom/media/fmp4/gonk/GonkAudioDecoderManager.cpp
@@ +97,5 @@
>    }
>  }
>  
> +status_t
> +GonkAudioDecoderManager::QueueSampleToOMX(mp4_demuxer::MP4Sample* aSample)

This function name seems to mismatch what it does (mDecoder->Input). 
No queue operation inside. It would be better to change the name.

::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp
@@ +26,5 @@
>  
> +nsresult
> +GonkDecoderManager::Input(mp4_demuxer::MP4Sample* aSample)
> +{
> +  // To maintain to order of the MP4Sample, it needs to queue samples in

... to *the* order of the MP4Sample...

@@ +27,5 @@
> +nsresult
> +GonkDecoderManager::Input(mp4_demuxer::MP4Sample* aSample)
> +{
> +  // To maintain to order of the MP4Sample, it needs to queue samples in
> +  // mQueueSample. And then the current input aSample.

What do you mean by "And then the current input aSample."

@@ +36,5 @@
> +
> +  for (uint32_t i = 0; i < len; i++) {
> +    rv = QueueSampleToOMX(mQueueSample.ElementAt(0));
> +    if (rv != OK) {
> +      break;

Should we return here?

@@ +40,5 @@
> +      break;
> +    }
> +    mQueueSample.RemoveElementAt(0);
> +  }
> +

It seems to make codes more easily readable to put the above codes into a function. Besides it is more likely what QueueSampleToOMX does.

@@ +83,5 @@
> +
> +nsresult
> +GonkDecoderManager::Flush()
> +{
> +  mQueueSample.Clear();

it might be better to have a lock to make sure mQueueSample will not be used when we clear it.

Comment 12

[Alfredo Yang (:alfredo)]

Attachment: handle_input_error

Comment 13

[Alfredo Yang (:alfredo)]

It is possible that OMX input buffer is running out. In old code, it returns an error to MDSM and stop playing.
To prevent this problem, this patch uses an queue to keep the samples when the OMX input buffer is not available. And send these queued samples next round.

Comment 14

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

Comment on attachment 8554970 [details] [diff] [review]
handle_input_error

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

Two minor suggestions to make codes simpler.

::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp
@@ +73,5 @@
> +      PerformFormatSpecificProcess(aSample);
> +    } else {
> +      tmp = sample;
> +    }
> +    rv = SendSampleToOMX(tmp);

It seems to make codes simpler to let PerformFormatSpecificProcess called in the SendSampleToOMX.

::: dom/media/fmp4/gonk/GonkMediaDataDecoder.h
@@ +69,5 @@
> +  // any sample be queued after EOS.
> +  nsTArray<nsAutoPtr<mp4_demuxer::MP4Sample>> mQueueSample;
> +
> +  // True when mQueueSample gets an empty MP4Sample.
> +  bool mEOS;

It would be better to change it to be mInputEOS.

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8554970 [details] [diff] [review]
handle_input_error

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

You're moving things around an inheritance hierarchy. I'd like to see some effort go into removing the inheritance hierarchy from this class at some point in the future. WMFMediaDataDecoder uses delegation instead of inheritance which makes it a lot less messy. We should also be trying to move toward lockless code rather than adding more monitors. With that said I don't want to hold you up right now.

::: dom/media/fmp4/gonk/GonkAudioDecoderManager.cpp
@@ +115,5 @@
> +        mp4_demuxer::Adts::GetFrequencyIndex(mAudioRate);
> +    bool rv = mp4_demuxer::Adts::ConvertSample(mAudioChannels,
> +                                               frequency_index,
> +                                               mAudioProfile,
> +                                               aSample);

Note: We want to move away from converting the audio samples to ADTS because this means that some HE-AAC streams don't play properly. You'll probably want to talk to jya about a follow up bug to fix this.

::: dom/media/fmp4/gonk/GonkMediaDataDecoder.h
@@ +61,5 @@
> +  // It sends MP4Sample to OMX layer. It must be overrided by subclass.
> +  virtual android::status_t SendSampleToOMX(mp4_demuxer::MP4Sample* aSample) = 0;
> +
> +  // It protects mQueueSample.
> +  ReentrantMonitor mMonitor;

It would make more sense to do all the updates to mQueueSample by dispatching to mTaskQueue instead of adding a monitor here. This is how WMFMediaDataDecoder::Input() avoids the need for locking.

Comment 16

[Alfredo Yang (:alfredo)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #15)
> Comment on attachment 8554970 [details] [diff] [review]
> handle_input_error
> 
> Review of attachment 8554970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You're moving things around an inheritance hierarchy. I'd like to see some
> effort go into removing the inheritance hierarchy from this class at some
> point in the future. WMFMediaDataDecoder uses delegation instead of
> inheritance which makes it a lot less messy. We should also be trying to
> move toward lockless code rather than adding more monitors. With that said I
> don't want to hold you up right now.

I fiile a bug 1127656 to remove the hierarchy later.

> 
> ::: dom/media/fmp4/gonk/GonkAudioDecoderManager.cpp
> @@ +115,5 @@
> > +        mp4_demuxer::Adts::GetFrequencyIndex(mAudioRate);
> > +    bool rv = mp4_demuxer::Adts::ConvertSample(mAudioChannels,
> > +                                               frequency_index,
> > +                                               mAudioProfile,
> > +                                               aSample);
> 
> Note: We want to move away from converting the audio samples to ADTS because
> this means that some HE-AAC streams don't play properly. You'll probably
> want to talk to jya about a follow up bug to fix this.
> 
> ::: dom/media/fmp4/gonk/GonkMediaDataDecoder.h
> @@ +61,5 @@
> > +  // It sends MP4Sample to OMX layer. It must be overrided by subclass.
> > +  virtual android::status_t SendSampleToOMX(mp4_demuxer::MP4Sample* aSample) = 0;
> > +
> > +  // It protects mQueueSample.
> > +  ReentrantMonitor mMonitor;
> 
> It would make more sense to do all the updates to mQueueSample by
> dispatching to mTaskQueue instead of adding a monitor here. This is how
> WMFMediaDataDecoder::Input() avoids the need for locking.

I will have a patch to correct it in bug 1127654 soon.

Comment 17

[Alfredo Yang (:alfredo)]

Attachment: handle_input_error

Minor fix from review comment. Carry r+.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de376de7ea00

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de376de7ea00