1573799 - Investigate effectiveness of OutputMediaStreams to keep OutputStreamManager::mGraph alive

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1572858 suggests that streams are being added to an MSG after it has passed the point of no return in shut down.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1571004#c7:

(In reply to Karl Tomlinson (:karlt) from comment #6)

I wonder what provides that the graph has not been destroyed at https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/media/mediasink/OutputStreamManager.cpp#252

https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/html/HTMLMediaElement.h#769-772

For that to function as intended, at least one OutputMediaStream needs to live
until the OutputStreamManager is discarded.

OutputMediaStreams are kept alive by HTMLMediaElement::mOutputStreams.
OutputMediaStreams can be removed from that array in HTMLMediaElement::AudioCaptureStreamChange(bool aCapture) but OutputStreamManager is notified only when mDecoder is still set.

Before mDecoder is reset DiscardFinishWhenEndedOutputStreams() is called, but this does not notify OutputStreamManager when !mOutputStreams[i].mFinishWhenEnded.

The seriousness of consequences of OutputStreamManager::mGraph shutting down before re-use depend on whether streams created on the shut-down graph can be connected to streams on a new graph.

Comment 1

[Andreas Pehrson [:pehrsons]]

(In reply to Karl Tomlinson (:karlt) from comment #0)

https://bugzilla.mozilla.org/show_bug.cgi?id=1572858 suggests that streams are being added to an MSG after it has passed the point of no return in shut down.

The only way media element capture gets started is if either of these are true:

1. getUserMedia with audioCapture, which is not per spec so not tested in WPT
2. mozCaptureStream or mozCaptureStreamUntilEnded on HTMLMediaElement, neither are per spec so not tested in WPT
3. MediaElementAudioSourceNode which is per spec and tested in WPT. Looking at what tests are using it, the ones in the log on bug 1572858 are after the assertion, so it seems like the culprit there lies elsewhere.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1571004#c7:

(In reply to Karl Tomlinson (:karlt) from comment #6)

I wonder what provides that the graph has not been destroyed at https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/media/mediasink/OutputStreamManager.cpp#252

https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/dom/html/HTMLMediaElement.h#769-772

For that to function as intended, at least one OutputMediaStream needs to live
until the OutputStreamManager is discarded.

OutputMediaStreams are kept alive by HTMLMediaElement::mOutputStreams.
OutputMediaStreams can be removed from that array in HTMLMediaElement::AudioCaptureStreamChange(bool aCapture) but OutputStreamManager is notified only when mDecoder is still set.

AudioCaptureStreamChange only comes into effect with getUserMedia({audio: {mediaSource: "audioCapture"}}) which is disabled by default. I'm not too concerned with issues related to audioCapture in the short term.

Before mDecoder is reset DiscardFinishWhenEndedOutputStreams() is called, but this does not notify OutputStreamManager when !mOutputStreams[i].mFinishWhenEnded.

If !mFinishWhenEnded the idea is that the SharedDummyStream keeps the graph alive until the OutputStream needs another track, through a new OutputStreamManager that re-fetches the same graph through GetInstance().

OTOH, if mFinishWhenEnded (and no other OutputStream has !mFinishWhenEnded) the idea is that the MediaDecoder stack, including OutputStreamManager shuts down (we ended!), and the new MediaDecoderStateMachine creates a new OutputStreamManager when needed. No streams attached to the old manager should then exist. GetInstance should not be able to return the old, shut-down, graph.

The seriousness of consequences of OutputStreamManager::mGraph shutting down before re-use depend on whether streams created on the shut-down graph can be connected to streams on a new graph.

Comment 2

[Karl Tomlinson (:karlt)]

Thanks for looking.

The specific situation I was questioning here is the AudioCaptureStreamChange case with !mFinishWhenEnded.

In that case, it looks like AudioCaptureStreamChange could remove the OutputMediaStream keeping the SharedDummyStream alive, but the OutputStreamManager would continue to stay alive.

(In reply to Andreas Pehrson [:pehrsons] from comment #1)

If !mFinishWhenEnded the idea is that the SharedDummyStream keeps the graph alive until the OutputStream needs another track, through a new OutputStreamManager that re-fetches the same graph through GetInstance().

If removing the Decoder disconnects the OutputStreamManager holding the old graph reference so that the OutputStreamManager is not used again, then there is no problem. It's not clear to me that the OutputStreamManager won't be used again, given the asynchronous nature of decoders adding tracks, but I don't have a good understanding of that.

AudioCaptureStreamChange only comes into effect with getUserMedia({audio: {mediaSource: "audioCapture"}}) which is disabled by default. I'm not too concerned with issues related to audioCapture in the short term.

OK. I'll link this to bug 1156472 so that we don't lose track of it, but fixing bug 1573102 would obsolete this anyway.

Comment 3

[Andreas Pehrson [:pehrsons]]

(In reply to Karl Tomlinson (:karlt) from comment #2)

Thanks for looking.

The specific situation I was questioning here is the AudioCaptureStreamChange case with !mFinishWhenEnded.

In that case, it looks like AudioCaptureStreamChange could remove the OutputMediaStream keeping the SharedDummyStream alive, but the OutputStreamManager would continue to stay alive.

Right. So it looks like there's a gap if the application stops the audioCapture track (AudioCaptureStreamChange is ultimately controlled from here) and again starts audioCapture, all while a media element were using the same decoder. But we're good, because when the OutputStreamManager becomes empty (no output streams), which is the case here, it gets cleaned up. Should a new output stream appear, a new OutputStreamManager will be created with a valid graph from the initiator of the capture. And if it wasn't empty, there would be another SharedDummyStream in the media element keeping it alive.

I do agree this is extremely convoluted right now with all this indirection, but I think once we ship captureStream per spec this will be properly cleaned up. This code is going through a phase right now. ;-)

Does this iron out the last wrinkles in your question?

(In reply to Andreas Pehrson [:pehrsons] from comment #1)

If !mFinishWhenEnded the idea is that the SharedDummyStream keeps the graph alive until the OutputStream needs another track, through a new OutputStreamManager that re-fetches the same graph through GetInstance().

If removing the Decoder disconnects the OutputStreamManager holding the old graph reference so that the OutputStreamManager is not used again, then there is no problem. It's not clear to me that the OutputStreamManager won't be used again, given the asynchronous nature of decoders adding tracks, but I don't have a good understanding of that.

AudioCaptureStreamChange only comes into effect with getUserMedia({audio: {mediaSource: "audioCapture"}}) which is disabled by default. I'm not too concerned with issues related to audioCapture in the short term.

OK. I'll link this to bug 1156472 so that we don't lose track of it, but fixing bug 1573102 would obsolete this anyway.

Comment 4

[Karl Tomlinson (:karlt)]

I was actually concerned about the OutputStreamManager not becoming empty in the case where it is not notified because mDecoder has been cleared on the media element. That could be a problem if the decoder could still add a track after Shutdown() has been called, but I don't know whether or not that can happen.

As you indicate, bug 1572858 has a different cause, and this bug is not a priority right now.

Comment 5

[Andreas Pehrson [:pehrsons]]

Right, so the question is if OutputStreamManager::AddTrack could happen after OutputStreamManager::Disconnect()?

Hmm, quite possibly.

MSG::GetInstance() should help. Also if the Disconnect() made the manager silently ignore future calls.