Add pull API for SourceMediaStreams

RESOLVED FIXED in mozilla17

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
Created attachment 639331 [details] [diff] [review]
proposed fix

Tell me what you think of this API.
Attachment #639331 - Flags: review?(rjesup)

Updated

5 years ago
Blocks: 691234
OS: Windows 7 → All
Hardware: x86_64 → All
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()?
Attachment #639331 - Flags: review?(rjesup) → review+
(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.
Amount of data: Yes, since if it's >20ms we can make multiple 10ms pulls from webrtc
Created attachment 639651 [details] [diff] [review]
v2

Adds a parameter to NotifyPull to give an estimate (an upper bound, really) of how much data should be pulled.
Attachment #639651 - Flags: review?(rjesup)
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?
Attachment #639651 - Flags: review?(rjesup) → review+
(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.
No longer blocks: 691234
Blocks: 771834
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ba830b7dca
https://hg.mozilla.org/mozilla-central/rev/20ba830b7dca
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.