933695 - [MediaRecorder] MediaStreamDestination does not change end state after playback finish

Status: RESOLVED WONTFIX

(Core :: Web Audio, defect, P3)

Description

[u459114]

Reproduce steps:
1. Visit https://rawgithub.com/randylin/mediaRecorder/master/MediaRecorder.html
2. Click on GetAudioContext button.
3. Click start button.
You will see MediaRecorder keep recording even if playback finished. 

Create a MeediaStreamDesination from AudioContext, associate a buffer with this object and set this MediaStream as source of MediaRecorder, and play.

Inside Gecko, webaudio module create three mediastreams
AudioNodeStream->AudioNodeStream->TrackUnionStream

While playback finished, only the first AudioNodeStream set track as ENDED(track->SetEnded()). The second ANS and TrackUnionStream have no idea on it at all. As a result, MediaRecorder will stay in recording state forever.

Comment 1

[u459114]

In AudioNodeStream::ProduceOutput, it should check the status of the track in mInputs(Source port). If the source track is ended(track->IsEnded()), set associated output track in AndioNodeStream::mBuffer into end state(by track->SetEnded()). 

We may refer logic in TrackUnionStream::CopyTrackData
http://dxr.mozilla.org/mozilla-central/source/content/media/TrackUnionStream.h#l245

Comment 2

[Jason Smith [:jsmith]]

Blocks a blocker.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #2)
> Blocks a blocker.

Turns out this will still be needed post landing of bug 924724 per discussion on that bug - this deals specifically with a shutdown hang that happens with MediaRecorder + AudioContext. bug 924724 deals with the more general case that's present right now for the shutdown hang.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

MediaStreamDestinationEngine::ProduceAudioBlock never sets *aFinished to true, so the stream will not end.

Should MediaStreamAudioDestinationNode.stream end as it source stream ends? Or it shouldn't since MediaStreamAudioDestinationNode can change its source node.

@Ehsan:
Can you provide you opinions?

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

@Josh:
Since Ehsan is away, can you provide your opinions?
Thx.

Comment 6

[u459114]

(In reply to Jason Smith [:jsmith] from comment #3)
> (In reply to Jason Smith [:jsmith] from comment #2)
> > Blocks a blocker.
> 
> Turns out this will still be needed post landing of bug 924724 per
> discussion on that bug - this deals specifically with a shutdown hang that
> happens with MediaRecorder + AudioContext. bug 924724 deals with the more
> general case that's present right now for the shutdown hang.
Sorry, I didn't explain clear enough in bug 924724.

After patch in 924724 land, shutdown hang fix for this case, audioContext + MediaRecorder, as well.
What still unfixed is: media recorder keep stay in recording state even if media track in AudioNodeStream is already in ENDED state.

Bug 924724 is not depend on this issue.

Comment 7

[Josh Matthews [:jdm]]

We discussed setting aFinished in bug 865257:
>Hmm, you can't actually do this here, since JS might keep a reference to this node and add more inputs
>to it in the future.  Which means that you can never set *aFinished to true in a meaningful way, until
>the engine is destroyed.

Comment 8

[Jason Smith [:jsmith]]

(In reply to C.J. Ku[:CJKu] from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #3)
> > (In reply to Jason Smith [:jsmith] from comment #2)
> > > Blocks a blocker.
> > 
> > Turns out this will still be needed post landing of bug 924724 per
> > discussion on that bug - this deals specifically with a shutdown hang that
> > happens with MediaRecorder + AudioContext. bug 924724 deals with the more
> > general case that's present right now for the shutdown hang.
> Sorry, I didn't explain clear enough in bug 924724.
> 
> After patch in 924724 land, shutdown hang fix for this case, audioContext +
> MediaRecorder, as well.
> What still unfixed is: media recorder keep stay in recording state even if
> media track in AudioNodeStream is already in ENDED state.
> 
> Bug 924724 is not depend on this issue.

Ah. In that case, I don't think this is a blocker. The workaround a developer can do here is use an onended callback on a HTMLMediaElement derived from the media stream that the MediaStreamAudioDestinationNode generates to determine when the stream ends. When that's known, they could stop the recording directly.

Note - This is similar to the problem cited in bug 911475.

Comment 9

[Karl Tomlinson (:karlt)]

The output of a MediaStreamAudioDestinationNode never finishes because the
node always has at least 1 channel of input, even with no nodes connected.

"Each AudioNode input has a specific number of channels at any given
time. This number can change depending on the connection(s) made to the
input. If the input has no connections then it has one channel which is
silent."

I wonder how TrackUnionStream handles the situation of all inputs removed and
then a new input added.

If destroying the stream is detectable downstream, and I assume it is through
MediaRecoder at least, then either MediaStreamAudioDestinationNode will need
to ensure that it is not garbage collected while it has a connected output or we need to do something so that its collection is not detectable.  Some of the issues in bug 916392 are similar.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Karl Tomlinson (:karlt) from comment #9)
> I wonder how TrackUnionStream handles the situation of all inputs removed and
> then a new input added.

https://dvcs.w3.org/hg/audio/raw-file/tip/streams/StreamProcessing.html 4.2
"When the autofinish attribute is true, then when all stream inputs are finished (including if there are no inputs), the stream will automatically enter the finished state and never produce any more output (even if new inputs are attached)."

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Karl Tomlinson (:karlt) from comment #9)
> The output of a MediaStreamAudioDestinationNode never finishes because the
> node always has at least 1 channel of input, even with no nodes connected.

Do we still need to fix this bug if the spec. says so?

Comment 12

[Maire Reavy [:mreavy]]

(In reply to jwwang from comment #11)
> (In reply to Karl Tomlinson (:karlt) from comment #9)
> > The output of a MediaStreamAudioDestinationNode never finishes because the
> > node always has at least 1 channel of input, even with no nodes connected.
> 
> Do we still need to fix this bug if the spec. says so?

We want to match the spec unless we think the spec is wrong;  if we think the spec is wrong, we want to update the spec.  I'm doing a needinfo to Paul (padenot) with his spec editor's hat on.

Comment 13

[(no longer active)]

https://dvcs.w3.org/hg/audio/raw-file/tip/streams/StreamProcessing.html is the wrong spec to look at, we don't implement it.

Comment 14

[(no longer active)]

Also, as Josh said, we cannot put this stream into finished state since more inputs may get connected to it in the future.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #13)
> https://dvcs.w3.org/hg/audio/raw-file/tip/streams/StreamProcessing.html is
> the wrong spec to look at, we don't implement it.
Is there a right spec to look for the right behaviors of streams?

Comment 16

[(no longer active)]

(In reply to jwwang from comment #15)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #13)
> > https://dvcs.w3.org/hg/audio/raw-file/tip/streams/StreamProcessing.html is
> > the wrong spec to look at, we don't implement it.
> Is there a right spec to look for the right behaviors of streams?

http://dev.w3.org/2011/webrtc/editor/getusermedia.html#mediastream

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Hi Ehsan:
What is your opinion? Do we still need to fix this bug or won't fix it?

Comment 18

[(no longer active)]

I'm not sure if there was ever a bug here, I think our behavior complies with the spec.  But C.J. filed this bug, so let's ask him if he thinks there's anything to be done here.

Comment 19

[u459114]

Thanks for your classification. Since it's desired behavior and  bug 924724 had landed, I think we don't need to do anything here.