Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change addStream to addTrack; add RTCRtpSender/RTCRtpReceiver #3

Closed
wants to merge 5 commits into from

Conversation

juberti
Copy link
Contributor

@juberti juberti commented Oct 15, 2014

Initial version of doohickeys. @alvestrand @stefhak @pthatcherg PTAL and see if you think this is going in the right direction. If so I will deal with the few formatting and link nits remaining.

Details:

@juberti juberti changed the title Add RTCRtpSender, RTCRtpSender, and friends Add RTCRtpSender, RTCRtpReceiver, and friends Oct 15, 2014
@alvestrand
Copy link
Contributor

Good to have this!

Needs a better cover note - the core part in this is the change from
AddStream to AddTrack.

I miss a paragraph on how the track's membership in streams is
signalled in the onaddtrack handler - there doesn't seem to be a
procedure specification for the firing of onaddtrack.

Something like this:

When a PeerConnection object detects that a track has been added by
the remote peer, it performs
the following steps:

- Let TRACK be a new MediaStreamTrack object
- Let STREAMS be the set of stream objects that the TRACK is a member of
- For each stream in STREAMS, construct an AddtrackEvent where 

(member variable listing)
- Fire the events at the PeerConnection

Section 5 of draft-ietf-mmusic-msid-07 needs to be consistent with this
document.

On 10/15/2014 08:23 AM, Justin Uberti wrote:

Initial version of doohickeys. @alvestrand
https://github.com/alvestrand @stefhak https://github.com/stefhak
@pthatcherg https://github.com/pthatcherg PTAL and see if you think
this is going in the right direction. If so I will deal with the few
formatting and link nits remaining.


    You can merge this Pull Request by running

git pull https://github.com/juberti/webrtc-pc master

Or view, comment on, or merge it at:

#3

    Commit Summary


Reply to this email directly or view it on GitHub
#3.

@@ -518,7 +509,7 @@
"component".</p>

<p>When a user agent has reached the point where a
<code><a>MediaStream</a></code> can be created to represent incoming
<code><a>MediaStreamTrack</a></code> can be created to represent incoming
components, the user agent MUST run the following steps:</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "components" (plural) still make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@pthatcherg
Copy link
Contributor

I left a few comments, some grammar. One was about what effect the "...
streams" variable has.

On Tue, Oct 14, 2014 at 11:23 PM, Justin Uberti [email protected]
wrote:

Initial version of doohickeys. @alvestrand https://github.com/alvestrand
@stefhak https://github.com/stefhak @pthatcherg
https://github.com/pthatcherg PTAL and see if you think this is going
in the right direction. If so I will deal with the few formatting and link

nits remaining.

You can merge this Pull Request by running

git pull https://github.com/juberti/webrtc-pc master

Or view, comment on, or merge it at:

#3
Commit Summary

  • Add RTCRtpSender, RTCRtpSender, and friends

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#3.

@pthatcherg
Copy link
Contributor

Overall, it looks very good, by the way.

On Wed, Oct 15, 2014 at 9:08 AM, Peter Thatcher [email protected]
wrote:

I left a few comments, some grammar. One was about what effect the "...
streams" variable has.

On Tue, Oct 14, 2014 at 11:23 PM, Justin Uberti [email protected]
wrote:

Initial version of doohickeys. @alvestrand
https://github.com/alvestrand @stefhak https://github.com/stefhak
@pthatcherg https://github.com/pthatcherg PTAL and see if you think
this is going in the right direction. If so I will deal with the few

formatting and link nits remaining.

You can merge this Pull Request by running

git pull https://github.com/juberti/webrtc-pc master

Or view, comment on, or merge it at:

#3
Commit Summary

  • Add RTCRtpSender, RTCRtpSender, and friends

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#3.

@juberti juberti changed the title Add RTCRtpSender, RTCRtpReceiver, and friends Change addStream to addTrack; add RTCRtpSender/RTCRtpReceiver Oct 15, 2014
@stefhak
Copy link
Contributor

stefhak commented Oct 16, 2014

Following up on Harald's part about steps when a PeerConnection object detects that a new track has been added by the remote peer: did we not conclude that the PC would not do this, but since the stream association is part of the addtrack event the application can add the track to the stream (and create the stream if it does not exist yet)?

And as a follow up: did we not say that the stream attribute on the addtrack event should be a list (for the case where a track i member of several MS's on the sending side at the time of doing pc.addTrack(track))?

(I may be remembering completely wrong here)

@juberti
Copy link
Contributor Author

juberti commented Oct 16, 2014

  • Only tracks added via pc.addTrack are communicated to the remote side.
  • The remote side can do whatever it wants regarding moving tracks around to different streams.
  • I think we do create a default stream if none is specified in signaling.
  • I think the stream attribute has to be a list (I will make the change, although I wonder if the complexity here is worth it - remind me why having tracks in multiple streams is useful?)

@juberti
Copy link
Contributor Author

juberti commented Oct 17, 2014

Fixed AddTrackEvent to handle multiple MediaStreams, and other CR comments. PTAL.

Fix the parameter descriptions for RTCAddTrackEvent
@juberti
Copy link
Contributor Author

juberti commented Oct 18, 2014

Merged in a link fix from @dontcallmedom and also cleaned up AddTrackEvent.

@juberti
Copy link
Contributor Author

juberti commented Oct 21, 2014

Any update on this PR? Would like to land this ahead of next week's meeting.

Also, I have this PR queued up to change updateIce -> setConfiguration.
juberti#2

@stefhak
Copy link
Contributor

stefhak commented Oct 21, 2014

The editor's are going to have it reviewed by Thu this week, hopefully it can be merged shortly after that.

referenced by the SDP
negotiation, creating a new ones if they do not exist. If no
MediaStreams are indicated in the SDP negotiation, a default MediaStream
is used.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section needs some work. The algorithm talks about several incoming components. That used to be the case when we were creating a MediaStream with several tracks; now we only have one. Step 3 (not in the diff), should be moved earlier, since it creates the MediaStreamTrack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I think I addressed these issues.

</dd>

<dt>readonly attribute sequence&lt;MediaStream&gt; streams</dt>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be a getter function since a sequence is passed by value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied what you did in your branch.

@adam-be
Copy link
Member

adam-be commented Oct 25, 2014

The stats-example uses pc.getRemoteStreams(). It needs to be updated as well.

@adam-be
Copy link
Member

adam-be commented Oct 25, 2014

I got a task to isolate the new section so it could be reviewed separately. I did some fixes as well that can be found here: https://github.com/adam-be/webrtc-pc/commits/juberti-rtpsr

@juberti
Copy link
Contributor Author

juberti commented Oct 27, 2014

Addressed all comments. PTAL.

@juberti
Copy link
Contributor Author

juberti commented Oct 29, 2014

@alvestrand @adam-be what are the next steps on this?

@stefhak
Copy link
Contributor

stefhak commented Oct 29, 2014

I know Adam has prepared a version of the spec with these parts included; I think the plan was to send it to the WG before the meetings this week.

<code>receiver</code>.</p>
</dd>

<dt>sequence&lt;MediaStream&gt; getStreams()</dt>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the getter comes from a change suggested by @adam-be , but it sounds to me that using a simple attribute with an array rather than a sequence would work just as well (i.e. readonly attribute MediaStream[] streams).

@alvestrand
Copy link
Contributor

Obsolete - PR is now #175

@alvestrand alvestrand closed this Jan 15, 2015
aboba added a commit that referenced this pull request Oct 22, 2015
Yet another typo (outbund)
aboba pushed a commit that referenced this pull request Mar 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants