1121342 - Regression in MSE/EME code...

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

This causes dom/media/test/test_eme_playback.html to timeout when MSE is used.

an hg bisect reveals:
The first bad revision is:
changeset:   247624:a41cce2dd408
parent:      247601:643589c3ef94
parent:      247623:a31444b37e97
user:        Carsten "Tomcat" Book <[e-mail]>
date:        Mon Jan 12 13:01:26 2015 +0100
summary:     merge fx-team to mozilla-central a=merge

The last commit made on Friday evening works fine.
changeset:   247621:8fbee3ef525f
user:        Wes Kocher <[e-mail]>
date:        Fri Jan 09 20:20:15 2015 -0800
summary:     Backout 42f2ce6de0c7 (bug 1117979) for xpcshell orange

Investigating to find out the actual culprit.

Comment 1

[Jean-Yves Avenard [:jya]]

The two branches merged have:
changeset:   247463:bb8d6034f5f2
user:        B2G Bumper Bot <[e-mail]>
date:        Fri Jan 09 19:33:50 2015 -0800
summary:     Bumping manifests a=b2g-bump

as common ancestor.

The FX branch is the one introducing the regression.

so:
hg bisect -b 643589c3ef94
hg bisect -g bb8d6034f5f2
The first bad revision is:
changeset:   247569:19b8c420b13a
user:        Bobby Holley <[e-mail]>
date:        Sun Jan 11 13:24:26 2015 -0800
summary:     Bug 1119456 - Make MP4Demuxer's blocking reads non-blocking and hoist blocking into callers with a hacky retry strategy. r=k17e

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Ensure the MDSM request a decode while waiting for the first frame.

Comment 3

[Jean-Yves Avenard [:jya]]

I see two issues at play here...

1- The Media Decoder State Machine request audio, this first attempt fails and we enter MediaDecoderStateMachine::OnNotDecoded, which then wait for data. Upon the promise being resolved, we reach MediaDecoderStateMachine::OnWaitForDataResolve. However, here we should be attempting to decode a frame (whatever we're waiting for). But the request is never done.

Attached patch fixing this.

But then there's issue #2.

2- While we are in waiting mode, the SourceBuffer has data appended to it ; and signal that it has data. Problem is that it doesn't have audio yet. so when MP4Reader::PopSample is called, InvokeAndRetry sees the same error twice it enters an EOS condition and no frame ever reached the MDSM.

It's late, will resume tomorrow, hoping the Bobby will have a solution for #2 by then

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> 1- The Media Decoder State Machine request audio, this first attempt fails
> and we enter MediaDecoderStateMachine::OnNotDecoded

Bug 1119456 should not be causing us to reject promises in situations where we didn't before. The idea is that there should be zero behavior change from the perspective of MP4Reader consumers.

> 2- While we are in waiting mode, the SourceBuffer has data appended to it ;
> and signal that it has data. Problem is that it doesn't have audio yet. so
> when MP4Reader::PopSample is called, InvokeAndRetry sees the same error
> twice

The SourceBufferResource::ReadInternal call should not return until we either (a) get enough bytes or (b) set mEnded.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #4)
> 
> Bug 1119456 should not be causing us to reject promises in situations where
> we didn't before. The idea is that there should be zero behavior change from
> the perspective of MP4Reader consumers.

should not is the key word isn't it ? :)


> 
> > 2- While we are in waiting mode, the SourceBuffer has data appended to it ;
> > and signal that it has data. Problem is that it doesn't have audio yet. so
> > when MP4Reader::PopSample is called, InvokeAndRetry sees the same error
> > twice
> 
> The SourceBufferResource::ReadInternal call should not return until we
> either (a) get enough bytes or (b) set mEnded.

The EME test is combined audio/video ; so the SourceBuffer notify that it has new data, but that may not be the one we need at that time.

Maybe we should just disable that EME test, since we don't support combined audio/video source buffer

Comment 6

[Jean-Yves Avenard [:jya]]

P1 as this is blocking all my queued patches

Comment 7

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> The EME test is combined audio/video ; so the SourceBuffer notify that it
> has new data, but that may not be the one we need at that time.

I don't follow what you mean here.

The distinction between audio and video aren't really relevant at this level. The point is that this loop does not return:

http://mxr.mozilla.org/mozilla-central/source/dom/media/mediasource/SourceBufferResource.cpp#66

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Re-search for Moof if an initial attempt to find it failed

Re-search for moof if an initial attempt to find it failed.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Re-search for Moof if an initial attempt to find it failed

Re-search for moof if an initial attempt to find it failed. Use existing return code

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8549590 [details] [diff] [review]
Re-search for Moof if an initial attempt to find it failed

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

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +3203,5 @@
>  
>      return 0;
>  }
>  
> +status_t MPEG4Source::lookForMoof() {

This code only gets called in the non-fragmented case (for fragmented, we have an AudioIterator, and we should be using the MoofParser). Shouldn't we kill all the lookForMoof stuff entirely?

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #10)
> Comment on attachment 8549590 [details] [diff] [review]
> Re-search for Moof if an initial attempt to find it failed
> 
> Review of attachment 8549590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
> @@ +3203,5 @@
> >  
> >      return 0;
> >  }
> >  
> > +status_t MPEG4Source::lookForMoof() {
> 
> This code only gets called in the non-fragmented case (for fragmented, we
> have an AudioIterator, and we should be using the MoofParser). Shouldn't we
> kill all the lookForMoof stuff entirely?

With EME, it's still called if the data is encrypted. No SampleIterator is created then.

> MP4Demuxer::Init()
>
> if (index->IsFragmented() && !mAudioConfig.crypto.valid)

I'm not familiar enough with eme to know if the sampleiterator could be used.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #11)
> With EME, it's still called if the data is encrypted. No SampleIterator is
> created then.
> 
> > MP4Demuxer::Init()
> >
> > if (index->IsFragmented() && !mAudioConfig.crypto.valid)
> 
> I'm not familiar enough with eme to know if the sampleiterator could be used.

Comment 13

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

Comment on attachment 8549590 [details] [diff] [review]
Re-search for Moof if an initial attempt to find it failed

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

We should not be fixing this code. It is an obsolete code path.

Comment 14

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

(In reply to Jean-Yves Avenard [:jya] from comment #11)
> I'm not familiar enough with eme to know if the sampleiterator could be used.

SampleIterator is the preferred path.

Comment 15

[Jean-Yves Avenard [:jya]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a801540e3d14
http://hg.mozilla.org/integration/mozilla-inbound/rev/0bbf63496d8d

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0bbf63496d8d
https://hg.mozilla.org/mozilla-central/rev/a801540e3d14

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8548892 [details] [diff] [review]
Ensure the MDSM request a decode while waiting for the first frame.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Youtube playback can fail to start.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Risk is low. This mostly affects MSE playback.
[String/UUID change made/needed]: None.

This approval request applies to both patches in this bug.

Comment 18

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/223b55babd02
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c9499d55d11

Comment 19

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7a8d1dd9fff3
https://hg.mozilla.org/releases/mozilla-beta/rev/dfbca180664d