Closed Bug 933695 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Web Audio, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: u459114, Assigned: jwwang)

Details

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.
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
Blocks a blocker.
blocking-b2g: --- → koi+
(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.
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?
Component: Video/Audio → Web Audio
Flags: needinfo?(ehsan)
@Josh: Since Ehsan is away, can you provide your opinions? Thx.
Flags: needinfo?(ehsan) → needinfo?(josh)
(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.
Blocks: MediaEncoder
No longer blocks: 924724
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.
Flags: needinfo?(josh)
(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.
blocking-b2g: koi+ → ---
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.
(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)."
Blocks: MediaRecording
No longer blocks: MediaEncoder
Blocks: 934818
(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?
(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.
Flags: needinfo?(paul)
https://dvcs.w3.org/hg/audio/raw-file/tip/streams/StreamProcessing.html is the wrong spec to look at, we don't implement it.
Also, as Josh said, we cannot put this stream into finished state since more inputs may get connected to it in the future.
(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?
(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
Priority: -- → P3
Hi Ehsan: What is your opinion? Do we still need to fix this bug or won't fix it?
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan) → needinfo?(cku)
Thanks for your classification. Since it's desired behavior and bug 924724 had landed, I think we don't need to do anything here.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(cku)
Resolution: --- → WONTFIX
No longer blocks: MediaRecording, 934818
Flags: needinfo?(paul)
You need to log in before you can comment on or make changes to this bug.