1168531 - Fix MediaCodecReader video playback problem

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

Bug 1154194 is going to add AskMediaCodecAndWait() usage to MediaCodecReader. It needs to be removed from MediaCodecReader. But just removing it causes playback regression. Bug 1132832 causes it. To fix it, MediaCodecProxy is also necessary to be updated.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Enable MediaCodecReader for playback

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Revert MediaCodecProxy::AskMediaCodecAndWait() usage

Comment 3

[Sotaro Ikeda [:sotaro]]

I changed my mind. It is better to be done in Bug 1168456 .

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> I changed my mind. It is better to be done in Bug 1168456 .

I changed my mind again, MediaCodecReader has several problems related video playback.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Make video decoder allocation async

Comment 6

[Sotaro Ikeda [:sotaro]]

By applying all patches, video handling still has a problem. Codec resource free is not handled correctly.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> By applying all patches, video handling still has a problem. Codec resource
> free is not handled correctly.

Same problem exists also on default master flame-kk's MediaCodecReader video playback.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Handle INVALID_OPERATION as decoding error

When the problem happens, MediaCodecReader::EnsureCodecFormatParsed() falled into infinite loop because of endless INVALID_OPERATION error handling.

Comment 9

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

Comment on attachment 8610750 [details] [diff] [review]
patch part 1 - Revert MediaCodecProxy::AskMediaCodecAndWait() usage

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

May I know why it is required to revert AskMediaCodecAndWait? 
This mechanism works well in Gonk PDM via MP4Reader and MediaFormatReader. 
Except one problem I just found during checking the flow of asking codec resource. When video element tries to enter dormant, ReleaseMediaResource will not be handled due to AskMediaCodecAndWait. I have created the bug 1168813 for it.

Comment 10

[Sotaro Ikeda [:sotaro]]

If MediaCodecReader could not get video codec, this function blocks forever. This could typically happen when multiple <video> elements exit on the system. This blocks task Queue thread forever.

CreateMediaCodecs() is called under MediaCodecReader::ReadMetadata(). Therefore, ReadMetadata() could be blocked forever if there are multiple multiple <video> elements exists. This should not happen. Bug 1132832 Comment 12 also have similar comment to AskMediaCodecAndWait().

After this bug is fixed, I am going to soon replace MediaCodecReader::ReadMetadata() by MediaCodecReader::AsyncReadMetadata(). AsyncReadMetadata() is async function, such possible long time block should not happen.

Comment 11

[Sotaro Ikeda [:sotaro]]

bwu, is there another question?

Comment 12

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

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> If MediaCodecReader could not get video codec, this function blocks forever.
> This could typically happen when multiple <video> elements exit on the
> system. This blocks task Queue thread forever.
Yeah. Would that blocked behavior cause any problems? Before getting codec resource, there should be no tasks waiting for being handled except the case in bug 1168813. 
> 
> CreateMediaCodecs() is called under MediaCodecReader::ReadMetadata().
> Therefore, ReadMetadata() could be blocked forever if there are multiple
> multiple <video> elements exists. This should not happen. Bug 1132832
> Comment 12 also have similar comment to AskMediaCodecAndWait().
Yes. IIUC, it is to implement async init() and then remove AskMediaCodecAndWait. 
> 
> After this bug is fixed, I am going to soon replace
> MediaCodecReader::ReadMetadata() by MediaCodecReader::AsyncReadMetadata().
> AsyncReadMetadata() is async function, such possible long time block should
> not happen.
Currently MediaFormatReader (replace MP4Reader) already supports AsyncReadMetadata and its decoding pipeline(gonk PDM, GonkMediaDataDecoder) uses AskMediaCodecAndWait as well. So far it works well. It still has the blocked situation as you mentioned.

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #12)
> (In reply to Sotaro Ikeda [:sotaro] from comment #10)
> > If MediaCodecReader could not get video codec, this function blocks forever.
> > This could typically happen when multiple <video> elements exit on the
> > system. This blocks task Queue thread forever.
> Yeah. Would that blocked behavior cause any problems? Before getting codec
> resource, there should be no tasks waiting for being handled except the case
> in bug 1168813. 

During blocking, TaskQueue thread can not run another task. Why do you think it is not a problematic? In this case, it could continue forever.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> > Yeah. Would that blocked behavior cause any problems? Before getting codec
> > resource, there should be no tasks waiting for being handled except the case
> > in bug 1168813. 

If we handle it as async, even bug 1168813 do not happen. And if is works as async, we could add another task in future and do not need to care about the blocking problem.

Removing blocking and make async is one of the important current gecko media playback framework's intent, isn't is?

Comment 15

[Sotaro Ikeda [:sotaro]]

I actually do not understand why AskMediaCodecAndWait() was started to be used for video codec instead of fixing the problem correctly.

Comment 16

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

(In reply to Sotaro Ikeda [:sotaro] from comment #15)
> I actually do not understand why AskMediaCodecAndWait() was started to be
> used for video codec instead of fixing the problem correctly.
AFAIK, originally ayang intended to remove/simplify those waiting and reserving coded resource codes from MediaDecoder and MDSM to readers. This codec reserving and waiting mechanism should be platform specific and would be better *not* to be exposed to the upper layer(MediaDecoder and MDSM).

Anyway, after finding the problem in bug 1168813 and the blocked behavior may cause the potential problem in the future as you mentioned, now I am clear and also agree with you that we should revert AskMediaCodecAndWait and use MediaPromise for the waiting resource which also simplifies the codes in MediaDecoder and MDSM. Awesome and thanks!

Comment 17

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

Comment on attachment 8610839 [details] [diff] [review]
patch part 2 - Make video decoder allocation async

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

Looks good to me....for video case. 
How about audio part? I didn't find the part to create audio codec. Did I miss something?

::: dom/media/omx/MediaCodecProxy.cpp
@@ +142,5 @@
> +MediaCodecProxy::AsyncAskMediaCodec()
> +{
> +  if ((strncasecmp(mCodecMime.get(), "video/", 6) != 0) ||
> +      (mResourceHandler == nullptr)) {
> +    return false;    

Remove trailing space.

Comment 18

[Benjamin Chen [:bechen]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #17)
> Comment on attachment 8610839 [details] [diff] [review]
> patch part 2 - Make video decoder allocation async
> 
> Review of attachment 8610839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me....for video case. 
> How about audio part? I didn't find the part to create audio codec. Did I
> miss something?

|AskMediaCodecAndWait()| for audio and the new function |AsyncAskMediaCodec()| for video.

Comment 19

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

(In reply to Benjamin Chen [:bechen] from comment #18)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #17)
> > Comment on attachment 8610839 [details] [diff] [review]
> > patch part 2 - Make video decoder allocation async
> > 
> > Review of attachment 8610839 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me....for video case. 
> > How about audio part? I didn't find the part to create audio codec. Did I
> > miss something?
> 
> |AskMediaCodecAndWait()| for audio and the new function
> |AsyncAskMediaCodec()| for video.
Yeah. Just found it. Thanks!
I thought AskMediaCodecAndWait is removed. This two function names look easily confused to me...
Maybe it would be better to use the same function to create audio and video codec. That would be more readable.

Comment 20

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

Comment on attachment 8610839 [details] [diff] [review]
patch part 2 - Make video decoder allocation async

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

Looks good to me!
Just some suggestions in comment 19.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #16)
> (In reply to Sotaro Ikeda [:sotaro] from comment #15)
> > I actually do not understand why AskMediaCodecAndWait() was started to be
> > used for video codec instead of fixing the problem correctly.
> AFAIK, originally ayang intended to remove/simplify those waiting and
> reserving coded resource codes from MediaDecoder and MDSM to readers. This
> codec reserving and waiting mechanism should be platform specific and would
> be better *not* to be exposed to the upper layer(MediaDecoder and MDSM).
> 
> Anyway, after finding the problem in bug 1168813 and the blocked behavior
> may cause the potential problem in the future as you mentioned, now I am
> clear and also agree with you that we should revert AskMediaCodecAndWait and
> use MediaPromise for the waiting resource which also simplifies the codes in
> MediaDecoder and MDSM. Awesome and thanks!

Thanks for the clarification!

Comment 22

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #19)
> (In reply to Benjamin Chen [:bechen] from comment #18)
> > (In reply to Blake Wu [:bwu][:blakewu] from comment #17)
> > > Comment on attachment 8610839 [details] [diff] [review]
> > > patch part 2 - Make video decoder allocation async
> > > 
> > > Review of attachment 8610839 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Looks good to me....for video case. 
> > > How about audio part? I didn't find the part to create audio codec. Did I
> > > miss something?
> > 
> > |AskMediaCodecAndWait()| for audio and the new function
> > |AsyncAskMediaCodec()| for video.
> Yeah. Just found it. Thanks!
> I thought AskMediaCodecAndWait is removed. This two function names look
> easily confused to me...
> Maybe it would be better to use the same function to create audio and video
> codec. That would be more readable.

I am going to put off it to another bug.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Make video decoder allocation async

Apply the comment.

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: roll-up patch

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/343ccb3b2576

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/343ccb3b2576