Closed Bug 1121342 Opened 9 years ago Closed 9 years ago

Regression in MSE/EME code...

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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 <cbook@mozilla.com>
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 <wkocher@mozilla.com>
date:        Fri Jan 09 20:20:15 2015 -0800
summary:     Backout 42f2ce6de0c7 (bug 1117979) for xpcshell orange

Investigating to find out the actual culprit.
Blocks: 1118589
The two branches merged have:
changeset:   247463:bb8d6034f5f2
user:        B2G Bumper Bot <release+b2gbumper@mozilla.com>
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 <bobbyholley@gmail.com>
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
Flags: needinfo?(bobbyholley)
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
(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.
Flags: needinfo?(bobbyholley)
(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
P1 as this is blocking all my queued patches
Priority: P2 → P1
(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
See Also: → 1121750
Re-search for moof if an initial attempt to find it failed.
Attachment #8549577 - Flags: review?(ajones)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Attachment #8548892 - Flags: review?(cpearce)
Re-search for moof if an initial attempt to find it failed. Use existing return code
Attachment #8549590 - Flags: review?(ajones)
Attachment #8549577 - Attachment is obsolete: true
Attachment #8549577 - Flags: review?(ajones)
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?
(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.
(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.
Flags: needinfo?(ajones)
Attachment #8548892 - Flags: review?(cpearce) → review+
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.
Attachment #8549590 - Flags: review?(ajones) → review+
(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.
Flags: needinfo?(ajones)
https://hg.mozilla.org/mozilla-central/rev/0bbf63496d8d
https://hg.mozilla.org/mozilla-central/rev/a801540e3d14
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8548892 - Flags: approval-mozilla-beta?
Attachment #8548892 - Flags: approval-mozilla-aurora?
Attachment #8548892 - Flags: approval-mozilla-beta?
Attachment #8548892 - Flags: approval-mozilla-beta+
Attachment #8548892 - Flags: approval-mozilla-aurora?
Attachment #8548892 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: