1132832 - [FFOS] MSE and EME Conformance Tests item 20 failed

Status: RESOLVED FIXED

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

Description

[Alfredo Yang (:alfredo)]

http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2015.html?timestamp=1423816812510

20. DASHLatency

Nightly browser is ok.

Comment 1

[Jean-Yves Avenard [:jya]]

Are you saying it fails when not using Nightly on fx-os ?

Normally, all YouTube conformance test should be green in nightly as of this morning

Comment 2

[Alfredo Yang (:alfredo)]

(In reply to Jean-Yves Avenard [:jya] from comment #1)
> Are you saying it fails when not using Nightly on fx-os ?
> 
> Normally, all YouTube conformance test should be green in nightly as of this
> morning

It fails on nightly gonk, nightly browser is ok. So it should be gonk PlatformDecoderModule's problem.

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: log.txt

OMX returns uninit error.

02-24 03:51:19.925 I/PRLog   ( 2911): 2015-02-24 08:51:19.937340 UTC - -1212298328[b1dd8900]: MediaSourceReader(b1210000)::virtual nsRefPtr<mozilla::MediaPromise<nsRefPtr<mozilla::VideoData>, mozilla::MediaDecoderReader::NotDecodedReason, true> > mozilla::MediaSourceReader::RequestVideoData(bool, int64_t): RequestVideoData(0, 0)
02-24 03:51:19.925 I/PRLog   ( 2911): 2015-02-24 08:51:19.937640 UTC - -1212298328[b1dd8900]: RequestVideoData skip=0 time=0
02-24 03:51:19.925 I/PRLog   ( 2911): 2015-02-24 08:51:19.937840 UTC - -1212298328[b1dd8900]: SchedulingUpdate(Video)
02-24 03:51:19.925 I/PRLog   ( 2911): 2015-02-24 08:51:19.938081 UTC - -1212298328[b1dd8900]: Update(Video) ni=1 no=1 iex=0 fl=0
02-24 03:51:19.925 D/GonkMediaDataDecoder( 2911): 0xb38b1b40 Input: sample=0xb17ddac0, force=0
02-24 03:51:19.925 D/MediaCodecProxy( 2911): MediaCodec has not been inited from input!
02-24 03:51:19.925 I/Gecko   ( 2911): [Child 2911] WARNING: GonkMediaDataDecoder failed to input data: file ../../../../../dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp, line 178
02-24 03:51:19.925 D/GonkMediaDataDecoder( 2911): Failed to input data err: -2147418113

Comment 4

[Alfredo Yang (:alfredo)]

MSE notifies MDSM OMX resource is acquired which is not ready yet. 


(gdb) bt
#0  mozilla::MediaDecoderStateMachine::NotifyWaitingForResourcesStatusChanged (this=0xb049c400) at ../../../dom/media/MediaDecoderStateMachine.cpp:1579
#1  0xb51f8418 in mozilla::MediaDecoder::NotifyWaitingForResourcesStatusChanged (this=0xb1403e00) at ../../../dom/media/MediaDecoder.cpp:1653
#2  0xb5243824 in mozilla::MediaSourceReader::OnTrackBufferConfigured (this=0xb049bc00, aTrackBuffer=0xb182a400, aTrackBuffer@entry=0xb182a420, aInfo=...) at ../../../../dom/media/mediasource/MediaSourceReader.cpp:677
#3  0xb524389c in mozilla::MediaSourceDecoder::OnTrackBufferConfigured (this=<optimized out>, aTrackBuffer=0xb182a420, aTrackBuffer@entry=0xb182a400, aInfo=...) at ../../../../dom/media/mediasource/MediaSourceDecoder.cpp:155
#4  0xb52438ee in mozilla::TrackBuffer::RegisterDecoder (this=this@entry=0xb182a400, aDecoder=aDecoder@entry=0xb0817d80) at ../../../../dom/media/mediasource/TrackBuffer.cpp:728
#5  0xb5247058 in mozilla::TrackBuffer::CompleteInitializeDecoder (this=0xb182a400, aDecoder=0xb0817d80) at ../../../../dom/media/mediasource/TrackBuffer.cpp:676
#6  0xb52481ac in nsRunnableMethodImpl<void (mozilla::TrackBuffer::*)(mozilla::SourceBufferDecoder*), mozilla::SourceBufferDecoder*, true>::Run (this=0xb089fea0) at ../../../dist/include/nsThreadUtils.h:361
#7  0xb46a8934 in nsThread::ProcessNextEvent (this=0xb386c110, aMayWait=<optimized out>, aResult=0xbed5dc8f) at ../../../xpcom/threads/nsThread.cpp:855
#8  0xb46bdc2c in NS_ProcessNextEvent (aThread=0xb386c110, aMayWait=aMayWait@entry=true) at /Users/Alfredo/mozilla/mozilla-central/xpcom/glue/nsThreadUtils.cpp:265
#9  0xb4875722 in mozilla::ipc::MessagePump::Run (this=0xb3801e50, aDelegate=0xbed5dde8) at ../../../ipc/glue/MessagePump.cpp:140
#10 0xb4861478 in MessageLoop::RunInternal (this=this@entry=0xbed5dde8) at ../../../ipc/chromium/src/base/message_loop.cc:233
#11 0xb4861492 in RunHandler (this=0xbed5dde8) at ../../../ipc/chromium/src/base/message_loop.cc:226
#12 MessageLoop::Run (this=0xbed5dde8) at ../../../ipc/chromium/src/base/message_loop.cc:200
#13 0xb54fc88a in nsBaseAppShell::Run (this=0xb2283640) at ../../widget/nsBaseAppShell.cpp:164
#14 0xb596d2c4 in XRE_RunAppShell () at ../../../toolkit/xre/nsEmbedFunctions.cpp:743
#15 0xb4875806 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0xb3801e50, aDelegate=0xbed5dde8) at ../../../ipc/glue/MessagePump.cpp:272
#16 0xb4861478 in MessageLoop::RunInternal (this=this@entry=0xbed5dde8) at ../../../ipc/chromium/src/base/message_loop.cc:233
#17 0xb4861492 in RunHandler (this=0xbed5dde8) at ../../../ipc/chromium/src/base/message_loop.cc:226
#18 MessageLoop::Run (this=this@entry=0xbed5dde8) at ../../../ipc/chromium/src/base/message_loop.cc:200
#19 0xb596d226 in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>, aGMPLoader=<optimized out>) at ../../../toolkit/xre/nsEmbedFunctions.cpp:580
#20 0xb6f98e14 in content_process_main (argc=6, argv=0xbed5e8e4) at ../../../ipc/app/../contentproc/plugin-container.cpp:211
#21 0xb6ecd4a4 in __libc_init (raw_args=0xbed5e8e0, onexit=<optimized out>, slingshot=0xb6f98e75 <main(int, char**)>, structors=<optimized out>) at bionic/libc/bionic/libc_init_dynamic.cpp:112
#22 0xb6f98cf4 in _start ()

Comment 5

[Alfredo Yang (:alfredo)]

Attachment: mse_wait_resource

Comment 6

[Alfredo Yang (:alfredo)]

This patch also fixes bug 1133396, 1133397, and 1133408.

Initializing a try run.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Alfredo Yang from comment #5)
> Created attachment 8576545 [details] [diff] [review]
> mse_wait_resource

wouldn't it be simpler to have TrackBuffer::InitializeDecoder wait until the decoder is actually ready?

the initialization is done in a dedicated task queue ; it wouldn't matter how long this takes or if it's blocking.
Ideally you would be using a new MediaPromise that TrackBuffer::InitilizeDecoder will wait on and continue where it left off once the hardware decoder is actually ready.

That wouldn't require such big change and would certainly be more elegant I think.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8576545 [details] [diff] [review]
mse_wait_resource

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

"do not notify MDSM before hardware resource is ready." this is not what your patch is doing...

MediaSourceReader::PrepareInitialization() is only called once, so you check once that your hardware is ready or not. It's the only place that would have notified the MDSM that your hardware resource was ready.

After that, you rely on the MDSM to poll MediaSourceReader::IsWaitingMediaResources() (done in MDSM::DecodeMetadata) until the resource is ready and continue to proceed then.
Should there be any error initializing the hardware, you would have no way of passing the information back to JS (updateend would have been fired already).

the MDSM will come out DECODER_STATE_WAIT_FOR_RESOURCES state once a new appendBuffer is called. Which will not happen until loadedmetadata is fired if the DASH player is waiting on this event to continue appending data. (this is not an issue with the YouTube test because they do a big init segment + media segment append) ; you will stall will some other DASH players.

::: dom/media/fmp4/SharedDecoderManager.cpp
@@ +13,5 @@
> +#define MSE_DEBUG(arg, ...) PR_LOG(GetMediaSourceLog(), PR_LOG_DEBUG, ("SharedDecoderManager(%p)::%s: " arg, this, __func__, ##__VA_ARGS__))
> +#else
> +#define MSE_DEBUG(...)
> +#endif
> +

this is not MSE, I don't think it should be called MSE_DEBUG

@@ +50,5 @@
>    {
> +    // SharedDecoderManager is owned by MediaSourceReader and MDSM in parent
> +    // decoder needs to be notified when hardware resource is ready; otherwise,
> +    // MDSM will be in DECODER_STATE_WAIT_FOR_RESOURCES state forever.
> +    mManager->mParentDecoder->NotifyWaitingForResourcesStatusChanged();

It's owned by MSE only if used with MSE, this isn't always true.

@@ +115,5 @@
> +    return false;
> +#endif
> +  }
> +
> +  return mDecoder->IsHardwareAccelerated() & mDecoder->IsWaitingMediaResources();

&& ?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8576545 [details] [diff] [review]
mse_wait_resource

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

I would prefer to not follow the Allocate/Release anti pattern here again. The act of waiting for resource contention to resolve should be handled by a separate object, it should not be shoehorned into the MediaDataDecoder interface.

Can we please refactor this so that we can just Shutdown() the decoder when we don't need it, and create it when we do? We should have some sort of ActivationObject or Factory that manages upon which we can wait for the decoder to become available.

Comment 10

[Alfredo Yang (:alfredo)]

Thanks for your feedback, chris and jya.

I'll come out another patch probably few days later since it'll be a significant refactory.

Comment 11

[Alfredo Yang (:alfredo)]

Attachment: 1_mediadatadecoder_refactory

Hi Chris, I try to remove the resource allocation from MediaDataDecoder. Although this is pretty much a draft, could you please give me some feedback?
Thank you.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8583727 [details] [diff] [review]
1_mediadatadecoder_refactory

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

That looks nicer at the MP4Reader/PlatformDecoderModule level. Thanks.

jya should also provide feedback here, as he is refactoring MP4Reader/MediaSourceReader/PlatformDecoderModule.h to improve our architecture. He is I believe working towards making MediaDataDecoder::Init() asynchronous, so your AskMediaCodecAndWait() call in GonkMediaDataDecoder will need to change in future.

jya can review your patch when it's ready.

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8583727 [details] [diff] [review]
1_mediadatadecoder_refactory

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

This is much better. Definitely the way to go: have the decoder manage its own readiness.

While I will have to rebase my changes, they will go very well with this.
As cpearce as indicated, MediaDataDecoder::Init() / MediaDecoderReader::ReadMetadata() will become asynchronous. But it will be easy for you to simply resolve the promise when AskMediaCodecAndWait completes.

r- however, due to my comment in MP4Reader::ReleaseMediaResources ; I don't see how that can work as it is.
If I'm mistaken, feel free to correct me!

::: dom/media/fmp4/MP4Reader.cpp
@@ +404,5 @@
>      // need to reinit the demuxer.
>      mDemuxerInitialized = true;
>    } else if (mPlatform && !IsWaitingMediaResources()) {
>      *aInfo = mInfo;
>      *aTags = nullptr;

We get here because mDemuxerInitialized was false. Following actions won't work (HasAudio() HasVideo() that relies on the demuxer being initialized)

This seems to be outside the scope of this change to change this.

why that change?

@@ +1046,5 @@
>    return
>  #if defined(XP_WIN)
>          mDormantEnabled &&
>  #endif
> +        true;

I don't think we need #define(MOZ_GONK_MEDIACODEK) || defined(XP_WIN) any longer.

mDormantEnabled should be a general pref on all platforms (with a value true by default)

Dormant should be supported on all platforms now (including android)

@@ +1064,5 @@
>    if (mVideo.mDecoder) {
> +    mVideo.mDecoder->Shutdown();
> +    mVideo.mDecoder = nullptr;
> +    mPlatform->Shutdown();
> +    mPlatform = nullptr;

There is no PlatformDecoderManager::Shutdown() any longer. Setting mPlatform to nullptr will be sufficient.

However, mPlatform won't be re-created ever.

So the video decoder can never be re-created.

When/Where will you re-create mPlatform?
How do you get back from Dormant then?

As mAreDecodersSetup is never cleared, EnsureDecoderSetup() won't do anything.

I don't think you need / want to clear mPlatform. Only release the decoder.

Comment 14

[Alfredo Yang (:alfredo)]

Thanks for feedbacks.
I'll update my patch for upcoming changes about MediaDataDecoder::Init() and MediaDecoderReader::ReadMetadata(). And I guess I need to rebase after bug 1146086 is fixed.

> ::: dom/media/fmp4/MP4Reader.cpp
> @@ +404,5 @@
> >      // need to reinit the demuxer.
> >      mDemuxerInitialized = true;
> >    } else if (mPlatform && !IsWaitingMediaResources()) {
> >      *aInfo = mInfo;
> >      *aTags = nullptr;
> 
> We get here because mDemuxerInitialized was false. Following actions won't
> work (HasAudio() HasVideo() that relies on the demuxer being initialized)
> 
> This seems to be outside the scope of this change to change this.
> 
> why that change?

While returning from dormant, mDemuxerInitialized should be true here, demuxer doesn't need to be reinitialize here. So HasAudio() and HasVideo() keep the original value before dormant. mPlatform and mDecoder will be re-created.  mDemuxerInitialized is false only when loading the video first time.


> @@ +1064,5 @@
> >    if (mVideo.mDecoder) {
> > +    mVideo.mDecoder->Shutdown();
> > +    mVideo.mDecoder = nullptr;
> > +    mPlatform->Shutdown();
> > +    mPlatform = nullptr;
> 
> There is no PlatformDecoderManager::Shutdown() any longer. Setting mPlatform
> to nullptr will be sufficient.
> 
> However, mPlatform won't be re-created ever.
> 
> So the video decoder can never be re-created.
> 
> When/Where will you re-create mPlatform?
> How do you get back from Dormant then?
> 
> As mAreDecodersSetup is never cleared, EnsureDecoderSetup() won't do
> anything.
> 
> I don't think you need / want to clear mPlatform. Only release the decoder.

Actually, I play safe here because mPlatform involves EME [1]. I'm not sure if EME needs to be able dormant or not. If EME needs to be dormant, does mPlatform need to be released?

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.cpp#465

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to Alfredo Yang from comment #14)

> Actually, I play safe here because mPlatform involves EME [1]. I'm not sure
> if EME needs to be able dormant or not. If EME needs to be dormant, does
> mPlatform need to be released?
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.
> cpp#465

You should never release mPlatform unless you know that MP4Reader is going to be shutdown and destroyed.
If this instance of MP4Reader is to be re-used (which will happen coming out of dormant) then you won't be able to recreate the decoder. While a video decoder will be still usable within the SharedDecoderManager, there are circumstances where the decoder will be shutdown and re-created.
Also, you won't be able to play audio coming out of dormant (as you can't recreate the decoder).
Once mPlatform is destroyed, it can't be recreated as it is.

And EME is now dormant capable.

Comment 16

[Jean-Yves Avenard [:jya]]

I suggest you don't wait for other changes to go in, depending on how urgent MSE is for FFOS. it may take a while to implement all those changes, and unlikely to be done for 40.. 41 at best

Comment 17

[Alfredo Yang (:alfredo)]

Attachment: remove_media_resource_api

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8589074 [details] [diff] [review]
remove_media_resource_api

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

You forgot WMF. WMFDecoderModule needs to be modified too.

What about OMX MediaDecoderReader implementation that makes use of similar architecture? could be done in a follow up bug

::: dom/media/fmp4/MP4Reader.cpp
@@ +1071,5 @@
>    }
>    if (mVideo.mDecoder) {
> +    mVideo.mDecoder->Shutdown();
> +    mVideo.mDecoder = nullptr;
> +    mAreDecodersSetup = false;

I think this is a bit ugly.
This is not equivalent as the first call to MP4Reader::Init

You will re-create a PlatformDecoderModule when it doesn't need to be re-created.

In EnsureDecodersSetup, we should check if mPlatform is already set and if so bypass the recreation of mPlatform.

::: modules/libpref/init/all.js
@@ +270,5 @@
>  // Whether we should play videos opened in a "video document", i.e. videos
>  // opened as top-level documents, as opposed to inside a media element.
>  pref("media.play-stand-alone", true);
>  
> +#if defined(XP_WIN) || defined(MOZ_WIDGET_GONK)

this can be removed too and make it the default on mac/ffmpeg as well.

set those values by default

Comment 19

[Alfredo Yang (:alfredo)]

Attachment: remove_media_resource_api

Comment 20

[Alfredo Yang (:alfredo)]

Attachment: remove_media_resource_api

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8590085 [details] [diff] [review]
remove_media_resource_api

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

great.
r=me with nit addressed.

Next, getting rid of IsWaitingMediaResources() :)

::: dom/media/fmp4/wmf/WMFMediaDataDecoder.cpp
@@ -166,5 @@
> -void
> -WMFMediaDataDecoder::ReleaseMediaResources()
> -{
> -  DebugOnly<nsresult> rv = mTaskQueue->FlushAndDispatch(
> -    NS_NewRunnableMethod(this, &WMFMediaDataDecoder::ProcessReleaseDecoder));

WMFMediaDataDecoder::ProcessReleaseDecoder is now unused and should be removed.

::: dom/media/omx/MediaCodecProxy.cpp
@@ +81,3 @@
>                                wp<CodecResourceListener> aListener)
>  {
> +  sp<MediaCodecProxy> codec = new MediaCodecProxy(aLooper, aMime, aEncoder, aListener);

80 columns format

Comment 22

[Alfredo Yang (:alfredo)]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> Comment on attachment 8590085 [details] [diff] [review]
> remove_media_resource_api
> 
> Review of attachment 8590085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> great.
> r=me with nit addressed.
> 
> Next, getting rid of IsWaitingMediaResources() :)
> 
> ::: dom/media/fmp4/wmf/WMFMediaDataDecoder.cpp
> @@ -166,5 @@
> > -void
> > -WMFMediaDataDecoder::ReleaseMediaResources()
> > -{
> > -  DebugOnly<nsresult> rv = mTaskQueue->FlushAndDispatch(
> > -    NS_NewRunnableMethod(this, &WMFMediaDataDecoder::ProcessReleaseDecoder));
> 
> WMFMediaDataDecoder::ProcessReleaseDecoder is now unused and should be
> removed.
> 
> ::: dom/media/omx/MediaCodecProxy.cpp
> @@ +81,3 @@
> >                                wp<CodecResourceListener> aListener)
> >  {
> > +  sp<MediaCodecProxy> codec = new MediaCodecProxy(aLooper, aMime, aEncoder, aListener);
> 
> 80 columns format

Thank you for review.

Comment 23

[Alfredo Yang (:alfredo)]

Attachment: remove_media_resource_api

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffc715db1260

Comment 27

[Alfredo Yang (:alfredo)]

Follow up bug 1153149 - Get rid of IsWaitingMediaResources() in PlatformDecoderModule

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f9cc47c52a9

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f9cc47c52a9