Closed Bug 1153149 Opened 9 years ago Closed 9 years ago

Get rid of IsWaitingMediaResources() in PlatformDecoderModule

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(1 file, 3 obsolete files)

Assignee: nobody → ayang
Attachment #8595166 - Flags: review?(jyavenard)
Comment on attachment 8595166 [details] [diff] [review]
remove_IsWaitingMediaResource_from_PDM

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

r=me with comment applied (you don't need mInited)

::: dom/media/fmp4/MP4Reader.h
@@ +218,5 @@
>      uint64_t mNumSamplesOutput;
>      uint32_t mDecodeAhead;
>      // Whether this stream exists in the media.
>      bool mActive;
> +    bool mInited;

It's a terrible name ! mInitialized maybe.

However, I don't see the point of having this member. mInited always mirror the existence mDecoder.
As error are fatals, if mDecoder->Init() failed, we will never again reach the code where we would have mDecoder set but Init()  failed.

if mDecoder == nullptr -> mInited = false
If mDecoder == something -> mInited = true.

So testing for mDecoder is enough.

If we assumed we could be in a condition where mDecoder is set but mDecoder->Init() failed, then simply set mDecoder to nullptr if mDecoder->Init() failed
Attachment #8595166 - Flags: review?(jya) → review+
Some questions for current codec reservation and notifications. I noticed currently GonkVideoDecoderManager is waiting codec reservation in [1] which is a blocking call. So if video codec is already occupied by others, ReadMetadata will be blocked. Would this cause some problems, like user cannot terminate that process or MediaDecoderStateMachine can not be shutdown complete?  

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp&case=true#111
(In reply to Blake Wu [:bwu][:blakewu] from comment #3)
> Some questions for current codec reservation and notifications. I noticed
> currently GonkVideoDecoderManager is waiting codec reservation in [1] which
> is a blocking call. So if video codec is already occupied by others,
> ReadMetadata will be blocked. Would this cause some problems, like user
> cannot terminate that process or MediaDecoderStateMachine can not be
> shutdown complete?  
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/
> GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp&case=true#111

The waiting codec reservation is on another thread, not MediaDecoderStateMachine thread. So the problem you stated won't happen.
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8595166 [details] [diff] [review]
> remove_IsWaitingMediaResource_from_PDM
> 
> Review of attachment 8595166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment applied (you don't need mInited)
> 
> ::: dom/media/fmp4/MP4Reader.h
> @@ +218,5 @@
> >      uint64_t mNumSamplesOutput;
> >      uint32_t mDecodeAhead;
> >      // Whether this stream exists in the media.
> >      bool mActive;
> > +    bool mInited;
> 
> It's a terrible name ! mInitialized maybe.

I'll change the name.

> 
> However, I don't see the point of having this member. mInited always mirror
> the existence mDecoder.
> As error are fatals, if mDecoder->Init() failed, we will never again reach
> the code where we would have mDecoder set but Init()  failed.
> 
> if mDecoder == nullptr -> mInited = false
> If mDecoder == something -> mInited = true.
> 
> So testing for mDecoder is enough.
> 
> If we assumed we could be in a condition where mDecoder is set but
> mDecoder->Init() failed, then simply set mDecoder to nullptr if
> mDecoder->Init() failed

I think mInited is required in current media resource management.
If there are two videos opening in the same page. The first video acquired the codec successfully. The second one will be blocked in the mDecoder->Init(). In this situation, the mDecoder is not nullptr and mDecoder->Init() is blocked before the first video releases the codec.
Then MediaDecoderStateMachine could get the wrong value from IsWaitingMediaResources().
(In reply to Alfredo Yang from comment #5)

> I think mInited is required in current media resource management.
> If there are two videos opening in the same page. The first video acquired
> the codec successfully. The second one will be blocked in the
> mDecoder->Init(). In this situation, the mDecoder is not nullptr and
> mDecoder->Init() is blocked before the first video releases the codec.
> Then MediaDecoderStateMachine could get the wrong value from
> IsWaitingMediaResources().

No.. That's not how IsWaitingMediaResources() is being called.

If mDecoder->Init() is blocking, then MP4Reader::ReadMetadata() will not return.
IsWaitingMediaResources() is only ever called before and after ReadMetadata() (now in MediaDecoderReader::CallReadMetadata())

As MP4Reader::ReadMetadata() is blocked, MP4Reader::IsWaitingMediaResources can't be called for that reader.

Testing mDecoder is sufficient.
Actually, on second thought... Can MP4Reader::IsWaitingMediaResources() ever returns true then ?

It doesn't appear that it's possible now. If mDecoder->Init() may not returned and be blocking ReadMetadata() , bwu comment is valid.

It will not block the MediaDecoderStateMachine however as it's done in a promise.
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Actually, on second thought... Can MP4Reader::IsWaitingMediaResources() ever
> returns true then ?

I try to modify codes as comments and found MediaDecoderStateMachine will be in WAITING_FOR_RESOURCES [1] when resuming from dormant.

When resuming from dormant, the mDecoder is nullptr so MP4Reader::IsWaitingMediaResources() returns true, then MediaDecoderStateMachine won't called ReadMetadata(). Without calling ReadMetadata(), decoder won't be initialized and MDSM gets stuck in WAITING_FOR_RESOURCES.
 
I think it can be solved by initializing codec in MP4Reader::PreReadMetadata(). Or other suggestion?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderReader.cpp#206
If as per your earlier comment mDecoder->Init() is blocking until the decoder is ready, you can just get rid of MP4Reader::IsWaitingMediaResources().

If it's not blocking then we can't get rid of MediaDataDecoder::IsWaitingMediaResources yet and you'll have to wait until bug 1148292 is complete. After this, the MP4Reader ReadMetadata() will always return a promise, in which case we will simply have to resolve the promise once the decoder is ready. And we won't need IsWaitingMediaResources() at all..
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> If as per your earlier comment mDecoder->Init() is blocking until the
> decoder is ready, you can just get rid of
> MP4Reader::IsWaitingMediaResources().

I'll remove MP4Reader::IsWaitingMediaResources().
Thank you for review.
Attachment #8595166 - Attachment is obsolete: true
Attachment #8595867 - Attachment is obsolete: true
Hi sheriff, please check in after bug 1154512.
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87329d2ed8ce
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.