771135 - Add pull API for SourceMediaStreams

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

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

Attachment: proposed fix

Tell me what you think of this API.

Comment 2

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

Comment on attachment 639331 [details] [diff] [review]
proposed fix

Review of attachment 639331 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaStreamGraph.h
@@ +109,5 @@
> +   * When a SourceMediaStream has pulling enabled, and the MediaStreamGraph
> +   * control loop is ready to pull, this gets called. A NotifyPull implementation
> +   * is allowed to call the SourceMediaStream methods that alter track
> +   * data. It is not allowed to make other MediaStream API calls, including
> +   * calls to add or remove MediaStreamListeners.

Is there any assumption about how much data is expected to be pulled?  If so, document here.  Also, a NotifyPull() method should not block normally (or for very long, if at all), I presume - perhaps worth mentioning.

@@ +410,5 @@
> +   * Enable or disable pulling. When pulling is enabled, NotifyPull
> +   * gets called on MediaStreamListeners for this stream during the
> +   * MediaStreamGraph control loop. Pulling is initially disabled.
> +   * Due to unavoidable race conditions, after a call to SetPullEnabled(false)
> +   * it is still possible for a NotifyPull to occur.

Is there some way to know when it's safe to destroy the object for NotifyPull()?

Comment 3

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

(In reply to Randell Jesup [:jesup] from comment #2)
> Is there any assumption about how much data is expected to be pulled?  If
> so, document here.  Also, a NotifyPull() method should not block normally
> (or for very long, if at all), I presume - perhaps worth mentioning.

Yes, I'll update the comment.

We could pass in the optimal amount of data to pull. It would be the difference between the already-buffered amount and the amount we want to commit to the audio stream in this iteration of the control loop. Is that useful?

> Is there some way to know when it's safe to destroy the object for
> NotifyPull()?

MediaStreamListeners are reference counted so that shouldn't be a problem.

Comment 4

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

Amount of data: Yes, since if it's >20ms we can make multiple 10ms pulls from webrtc

Comment 5

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

Attachment: v2

Adds a parameter to NotifyPull to give an estimate (an upper bound, really) of how much data should be pulled.

Comment 6

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

Comment on attachment 639651 [details] [diff] [review]
v2

r+ since this does what it intends.

I assume this will look for 30ms on the first pull, and once it has enough buffered we'll get pulls that amount to asking for 10ms more every ~10ms barring something blocking it.  (I'm not sure I followed all the logic behind the time and blocking calcs.

I need to verify webrtc won't block or substitute made-up data (it actually probably does, so we may need to have a way to check how much it has), and how the "startup"/fill-the-buffers time is handled between the two.

What's the minimum to supply to avoid a glitch?  10ms?  Or the amount desired?

Comment 7

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

(In reply to Randell Jesup [:jesup] from comment #6)
> I assume this will look for 30ms on the first pull, and once it has enough
> buffered we'll get pulls that amount to asking for 10ms more every ~10ms
> barring something blocking it.  (I'm not sure I followed all the logic
> behind the time and blocking calcs.

That's about right.

> What's the minimum to supply to avoid a glitch?  10ms?  Or the amount
> desired?

The amount passed in. If you supply less, we'll have to pause the stream temporarily --> glitch.

Comment 8

[Anant Narayanan [:anant]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20ba830b7dca

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20ba830b7dca