Last Comment Bug 771135 - Add pull API for SourceMediaStreams
: Add pull API for SourceMediaStreams
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks: 771834
  Show dependency treegraph
 
Reported: 2012-07-05 07:06 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2012-07-20 21:03 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (7.30 KB, patch)
2012-07-05 07:07 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
v2 (11.95 KB, patch)
2012-07-06 06:58 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-05 07:06:06 PDT

    
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-05 07:07:01 PDT
Created attachment 639331 [details] [diff] [review]
proposed fix

Tell me what you think of this API.
Comment 2 Randell Jesup [:jesup] 2012-07-06 00:27:08 PDT
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) (Exited; email my personal email if necessary) 2012-07-06 01:07:46 PDT
(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] 2012-07-06 06:30:07 PDT
Amount of data: Yes, since if it's >20ms we can make multiple 10ms pulls from webrtc
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-06 06:58:58 PDT
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.
Comment 6 Randell Jesup [:jesup] 2012-07-06 13:40:17 PDT
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) (Exited; email my personal email if necessary) 2012-07-07 03:45:20 PDT
(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 9 Ryan VanderMeulen [:RyanVM] 2012-07-20 21:03:15 PDT
https://hg.mozilla.org/mozilla-central/rev/20ba830b7dca

Note You need to log in before you can comment on or make changes to this bug.