Re: Proposal: Remove stopped transceivers to prevent resource exhaustion

On 2/15/19 5:03 PM, Henrik Boström wrote:
> As always, this is more complicated than I expected.
>
> On Fri, Feb 15, 2019 at 6:17 PM youenn fablet <yfablet@apple.com 
> <mailto:yfablet@apple.com>> wrote:
>
>>     On Feb 15, 2019, at 3:02 AM, Henrik Boström <hbos@google.com
>>     <mailto:hbos@google.com>> wrote:
>>     ... This prevents the transceivers, and thus the associated
>>     senders/receivers and tracks, from being garbage collected. A
>>     stress test running Chrome that did add/removeTrack a lot ran out
>>     of memory when we switched from Plan B to Unified Plan due to
>>     this resulting in a few thousand transceivers in less than a
>>     minute. (I don't know the true footprint, but even if each
>>     transceiver just has a single video frame in its buffer of
>>     1280x720x4, that's several megabytes, multiplied by thousands of
>>     transceivers => gigabytes of data).
>
>     I agree with the concern.
>     That said, implementations could probably do some clean-up when a
>     transceiver is stopped so that it becomes very lightweight.
>     Is there anything preventing implementations to do so?
>
>
> Good point. The transceiver is irreversibly stopped, and its track is 
> irreversibly ended. I think it may be lightweight, but only if we 
> don't have to store the a frame on the ended track.
> What happens if you attach an ended track to a video element? Does it 
> show an the last received frame or no frame (black)?


https://jsfiddle.net/jib1/4cxpLu08/ shows neither Chrome nor Firefox 
show a frame today on re-attach, but see 
https://github.com/w3c/mediacapture-main/issues/555 for a discussion 
that might affect that.


> This may be a central question to the importance or non-importance of 
> the resource issue (and whether the OOM was a Chrome bug or a spec 
> concern).
>
>
> On Fri, Feb 15, 2019 at 7:06 PM Jan-Ivar Bruaroey <jib@mozilla.com 
> <mailto:jib@mozilla.com>> wrote:
>
>     Thanks Henrik for a good summary! I agree we should do something,
>     but I'd caution against big changes. Comments inline.
>
>     On 2/15/19 6:02 AM, Henrik Boström wrote:
>>     ... TL;DR: Keeping transceivers around prevents garbage
>>     collection of a possibly ever-growing list of tracks. Can we stop
>>     doing that?
>>
>>     If an application does addTrack() and removeTrack() with
>>     renegotiations in-between, the list of m= sections and
>>     transceivers will grow over time.
>
>     This will remain true. Developers who operate with the
>     addTrack()/removeTrack() symmetric mental model, will continue to
>     experience this side-effect. transceiver.stop() does not rescue
>     that mental model, since it stops both directions.
>
>     So the short answer to your "Can we stop doing that?" question is
>     "probably not".
>
>
> Good point. I don't think we'll fix the problem for people who have 
> the wrong mental model. Stopping a transceiver would have to be a 
> conscious act, where you don't stop a transceiver that may be used for 
> receiving.
>
>     That said, I agree we should fix the main side-effect of
>     transceiver.stop() you point out: that it holds on to objects long
>     after the underlying machinery it's supposed to represent (the
>     network negotiation slot) has been repurposed. Transceivers were
>     meant to be true to the underlying behaviors, and this fails that.
>
>>     ... Related issues/questions:
>>
>>      1. If you care about mids of stopped transceivers you'd have to
>>         record them yourself.
>>      2. There is no "onstopped" event in case a transceiver is
>>         stopped due to setRemoteDescription(). Should we add one, or
>>         is it enough that the application checks whether the
>>         transceiver still exists by querying getTransceivers() in
>>         track.onmute?
>>
>     `mute` only fires remotely AFAIK. However, transceiver.stop() [1]
>     does reliably fire `ended` on transceiver.receiver.track on both
>     ends. [2] But in neither case are you given a pointer to the
>     transceiver.
>
> Yes, "onended" is the one we want not "onmute". Do you need the 
> transceiver pointer though? When you add a listener to "onended" you 
> know the transceiver already (or you can look it up in the pc using 
> the track).
> You can do this:
>
> function onstoppedPromise(pc, transceiver) {
>   return new Promise(resolve => {
> transceiver.receiver.track.onended => {
> resolve(transceiver);
>     }
>   });
> }
> pc.ontrack = e => 
> onstoppedPromise(e.transceiver).then(processEndedTransceiver);

That works!


>>     Please discuss.
>
>     I think you hit on the most confusing situation: where the other
>     side stopped your transceiver.
>
>     The current spec API for this is to examine the boolean
>     `transceiver.stopped`, once you return to "stable". I think that
>     works, has a couple of years of this working group behind it,
>     Firefox implements it, and it has neither of the issues you mention.
>
>     With the change you propose, the transceiver would effectively
>     disappear from all lists we maintain for developers, before
>     developers get a chance to react. Finding out which transceiver(s)
>     were stopped might be hard. Ironically, the `transceiver.stopped`
>     API would still exist, but developers would have to cache the list
>     ahead of time to use it.
>
>     How about instead:
>
>       * we remove the stopped transceiver on subsequent renegotiation,
>         when its m-line is reused?—If we're clever, we may even be
>         able to insert the new transceiver that inherits the m-line,
>         in its place, preserving the index order of the other
>         transceivers in getTransceivers().
>
>     Thoughts?
>
> I see the benefit of keeping the transceiver around during 
> negotiation; it might be nice to have an easily accessible list of 
> transceivers that is complete and comparable to the SDP, including 
> stopped ones.
>
> This is where it gets hairy:
> > /If we're clever, we may even be able to insert the new transceiver 
> that inherits the m-line, in its place, preserving the index order of 
> the other transceivers in getTransceivers()/
> The "new transceiver" might not be newly created, it might be a 
> transceiver that was created (at index /n/) and is now tied to a 
> previous m-line, say index /i/. Do we move the transceiver?


Good point. m-lines become eligible for re-use on transition to "stable" 
state, but there's a window between then and the previous "stable" state 
where if addTransceiver() were to be called would create transceivers 
that should get first dibs on these m-lines.

What if we instead removed stopped transceivers in 
addTransceiver()/addTack()? I.e. we'd "overwrite" stopped transceivers 
with new ones, mimicking what should happen at the m-line level once we 
renegotiate.

This would seem to preserve indices, IF we want to go that way. I do 
worry about it not being POLA though.

> A lot boils down to what we want to be true about transceiver indices: 
> what do we want to be true?
>
> I believe the statement "the transceiver index is the SDP m-line 
> index" is /not true/ when both endpoints add transceivers before 
> negotiating, and possibly other edge cases. What /is true/ is "the 
> transceivers are listed in insertion order". If you add in-place 
> insertion of recycled m-sections, or move transceivers around, you 
> would sacrifice what was always true (insertion order) in order to 
> maintain support for what was sometimes true but not always 
> (transceiver index matches SDP). I'm not sure maintaining this 
> illusion is worth the complexity, especially when mid is what you 
> /should/ use when comparing SDP or the other endpoint's set of 
> transceivers.
>
> Here's a third option:
>
>   * Stopping a transceiver does not immediately remove it from the set
>     of transceivers, but when the signaling state goes from any
>     non-stable state to stable, all stopped transceivers are removed.
>
> It doesn't try to make the index match (if you can't guarantee, don't 
> try) but it maintains insertion order, and stopping a transceiver with 
> stop() or setRemoteDescription(offer) does not immediately make it 
> disappear. But if you are the offerer, transceivers stopped by the 
> answer would disappear when you do setRemoteDescription(answer).

Yeah so this would explicitly not solve the case I think would be most 
surprising, where the other side stopped us.

We should probably just pick what's simpler.

.: Jan-Ivar :.
> Is that better or worse?
>
>     .: Jan-Ivar :.
>
>     [1] http://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-stop
>     [2] https://blog.mozilla.org/webrtc/rtcrtptransceiver-explored/
>