832881 - Remote streams buffer when their output is blocked (media.pause())

Status: RESOLVED WORKSFORME

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

Description

[Randell Jesup [:jesup] (needinfo me)]

This is caused by the same issue that caused problems with input streams (bug 822956).  We need to use a TrackUnion stream wrapping a SourceMediaStream to disable blocking.

My proposed solution is to add another category of MediaStream to the current SourceMediaStream and TrackUnion MediaStream, a RealtimeSourceMediaStream.  This will be created in a similar manner: nsDOMMediaStream::CreateRealtimeStream(...), like CreateTrackUnionStream().  Unlike a plain TrackUnion stream, AsSourceStream() will return the wrapped SourceMediaStream.

This implementation can be used anywhere a non-blocking realtime stream is needed, and would probably replace the similar nsDOMUserMediaStream in MediaManager.cpp.  I tried to implement something similar to nsDOMUserMediaStream for MediaPipelines, but due to the amount of type collapsing done as things are passed around and converted (and because it deals with both MediaStreams from the PeerConnection, and MediaStreams from gUM, and because the actual internal object is exposed and used via GetStream()) the patch and code became confusing and unwieldy; this is much cleaner.

Note: the current patch does not yet replace nsDOMUserMediaStream.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add RealtimeMediaStreams and use for Remote PeerConnection streams

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 704462 [details] [diff] [review]
Add RealtimeMediaStreams and use for Remote PeerConnection streams

This is tested and seems to work well - pausing video elements in the loopback pc_test.html test and then un-pausing them causes an immediate jump to the current video.

Note: I haven't tested the unit tests yet; I'll do that before any checkin.  I doubt they'll be affected.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

The alternative to this patch (other than a rather painful rework of the MediaPipeline code, unless there's an easy way I missed) would be to add non-blocking support to SourceMediaStreams directly, and eliminate the need for a TrackUnion stream to wrap it.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Making AsSourceStream() return a stream that's not 'this' seems like a big problem. Everything that currently uses AsSourceStream assumes it's simply a checked downcast.

I would like to understand why doing what we did for gUM is difficult for PeerConnectionImpl. We should probably talk about this tomorrow.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

These don't include test runs - they're for standard8 and florian to test.  This *may* help with general delay and sync (and may not); they *should* stop any use of pause()/etc from adding permanent delay.

Try (using the same base as previous Trys on bug 832567 which was known to be good): 
https://tbpl.mozilla.org/?tree=Try&rev=8275fb6bcce6

Try using an up-to-date base off m-i:
https://tbpl.mozilla.org/?tree=Try&rev=0c97f4411637

roc: I'd love to find a simple way to do this outside MediaStream, because of the exposure of GetStream() it seems that it's tough to wrap it in a reasonable subclass, though maybe by making much more wholesale changes to MediaPipeline/PeerConnectionMedia it might become easier.  (In fact I'd think so, but in practice it seemed to be a nightmare, especially when the hacks to support C++ unit tests impinged.)

I certainly haven't checked all the uses of AsSourceStream() or tried any other media scenarios other than PeerConnection; I just got it to work and was hoping it would help Marc & Florian to get it up and Tries built.

If that's the only concern, we could change AsSourceStream to WrappedSourceStream, and make all the callers in MediaPipeline to do "SourceMediaStream *source = stream->AsSourceStream(); if (!source) source = stream->WrappedSourceStream()"  This side-steps the issues with subclassing it outside of MediaStream without changing the assumptions around AsSourceStream()

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add RealtimeMediaStreams and use for Remote PeerConnection streams

This version doesn't touch AsSourceStream, so it can remain an "is-a" function.

I added instead SourceStream(), which may return a wrapped stream and is not an "is-a" function, and generally would be used by external code interacting with streams.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Basically a RealtimeMediaStream is a TrackUnionStream with a SourceMediaStream attached to it and a getter for that SourceMediaStream.

I don't understand why that's so much easier to use than an nsDOMMediaStream subclass with getters for both a TrackUnionStream and a SourceMediaStream. It looks to me like almost everywhere you call SourceStream(), you also have an nsDOMMediaStream that you could downcast (checked or not) and get an embedded SourceMediaStream from. It seems to me that the places that call SourceStream() know whether they're dealing with a "realtime" stream or not.

Embedding an additional MediaStream inside a MediaStream and adding SourceMediaStream() to every MediaStream seems like an ugly way to solve this problem.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

[paraphrase from IRC]
I will try to revive my other patch. However, I found that there was a lot more pain down that route than I anticipated due to some of the ways nsDOMMediaStream was being played with, especially when you factored in the "fake" mediastreams used for the C++ unit tests (since we have trouble accessing the real ones due to horrible linkage issues). I did go down that route first. 

I'll make another attempt at it and upload the patch for comparison (even if I can't get it to run successfully, which was part of why I abandoned that approach, along with the breaking all the Unit tests at compile time)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One thing that might possibly help is to add a getter for MediaStream::mWrapper (main thread only). Then, for code on the main thread you can get from a MediaStream to its nsDOMMediaStream (if there is one) and then dynamically check whether it's an nsDOMMediaStream with a separate SourceMediaStream inside it.

Another thing that might help is some kind of UserData API for MediaStreams. But getting lifetimes right would be a bother.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

This may have been resolved somewhere along the way, since Pausing a remote video element in pc_test.html and then unpausing doesn't leave any delay.