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

Add RTCRtpTransceiver and PeerConnection.addMedia. #293

Merged
merged 18 commits into from
Oct 24, 2015

Conversation

pthatcherg
Copy link
Contributor

This is intended to replace createRtpSender (#271) and createRtpReceiver (#279) with something more powerful and complete. It also fixes #187.

Needs list discussion, and I'd like to present on it at the f2f.

@@ -2559,6 +2557,17 @@
</ol>
</dd>

<dt>RTCRtpSender addMedia (DOMString kind, RTCRtpTransceiverInit init)</dt>
Copy link
Member

Choose a reason for hiding this comment

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

RTCRtpTransceiver as return value?
First argument: (MediaStreamTrack or DOMString) media?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and yes.

@adam-be
Copy link
Member

adam-be commented Sep 9, 2015

I couldn't get my microphone working during this session on the f2f meeting.

As I understand this PR, there's API for the A-side to add media and inspect existing transceivers. Have you thought anything about how the B-side API looks?

@@ -2446,6 +2424,26 @@
defined and the order does not have to be stable between calls.</p>
</dd>

<dt>sequence&lt;RTCRtpTranseceivers&gt; getMedia()</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 should probably be Transceiver (here and in the rest of the text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, fixed it.

@martinthomson
Copy link
Member

Consensus from the meeting was that the name was bad. I don't think that there was wide agreement on a name, but I recall addMediaSection being mentioned. If that is the name then RTCTransceiver could - and maybe should - be named RTCMediaSection.

@jan-ivar
Copy link
Member

RTCMediaLine?

@pthatcherg
Copy link
Contributor Author

Yes, addMedia is a bad name. But how is RTCRtpTransceiver? If it's
palatable, then we could simply call the method to create one
createRtpTransceiver. It's a bit advanced, but it exists mostly for
advanced use cases anyway.

On Wed, Sep 16, 2015 at 10:28 AM, Martin Thomson notifications@github.com
wrote:

Consensus from the meeting was that the name was bad. I don't think that
there was wide agreement on a name, but I recall addMediaSection being
mentioned. If that is the name then RTCTransceiver could - and maybe
should - be named RTCMediaSection.


Reply to this email directly or view it on GitHub
#293 (comment).

@pthatcherg
Copy link
Contributor Author

What was mentioned on one my slides was the full "bear hug":
addSdpMediaSection with RTCSdpMediaSection. But it wasn't met with much
approval.

On Wed, Sep 16, 2015 at 10:28 AM, Martin Thomson notifications@github.com
wrote:

Consensus from the meeting was that the name was bad. I don't think that
there was wide agreement on a name, but I recall addMediaSection being
mentioned. If that is the name then RTCTransceiver could - and maybe
should - be named RTCMediaSection.


Reply to this email directly or view it on GitHub
#293 (comment).

@alvestrand
Copy link
Contributor

Met with approval in Seattle (modulo name bikeshed). Should be reviewed and merged.

@alvestrand
Copy link
Contributor

Waiting for naming discussions to conclude. Also, Travis errors.

@pthatcherg
Copy link
Contributor Author

FYI, I have change the name in the PR to createRtpTransceiver until we come
up with a better one.

On Wed, Sep 16, 2015 at 10:50 AM, Peter Thatcher pthatcher@google.com
wrote:

Yes, addMedia is a bad name. But how is RTCRtpTransceiver? If it's
palatable, then we could simply call the method to create one
createRtpTransceiver. It's a bit advanced, but it exists mostly for
advanced use cases anyway.

On Wed, Sep 16, 2015 at 10:28 AM, Martin Thomson <notifications@github.com

wrote:

Consensus from the meeting was that the name was bad. I don't think that
there was wide agreement on a name, but I recall addMediaSection being
mentioned. If that is the name then RTCTransceiver could - and maybe
should - be named RTCMediaSection.


Reply to this email directly or view it on GitHub
#293 (comment).

@@ -2559,6 +2560,25 @@
</ol>
</dd>

<dt>RTCRtpTransceiver createRtpTransceiver(MediaStreamTrack or
DOMString track_or_kind, RTCRtpTransceiverInit init)</dt>

Copy link
Member

Choose a reason for hiding this comment

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

The union type MediaStreamTrack or DOMString needs to be wrapped in parenthesis, i.e. (MediaStreamTrack or DOMString) track_or_kind

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 Author

Sorry about all the typos. I guess I was a little too rushed when writing this. Hopefully you found all of them.

@stefhak
Copy link
Contributor

stefhak commented Oct 20, 2015

What happens if you create two transceivers (of same type), and two tracks, and then receive one or two tracks (sort of like SSRCs) in RTP? How would that be wired up?
Also, #283 had an event fired when unknown media arrived at any time (i.e. also if there was no ongoing O/A). That is not supported here - are we fine with removing that? Asking @martinthomson.
Finally: if I were to implement this, I think I would need some more normative text. An example showing how the Transceiver can be used to handle early media is good, but the spec must be clear enough to implement.
To be clear: I think we should merge this PR, but knowing there is more work to be done speccing things out.

@pthatcherg
Copy link
Contributor Author

I don't remember what the plan for completely unknown media is. I'd have
to go read the f2f minutes/notes to remember. Martin, do you remember?

As for normative text, I'll add some in the addTransceiver method saying
that the receiver can receive media before setRemoteDescription is called
with an answer, along with the example which I'll add.

If you create two transceivers of the same type, they'll have different
MIDs, and they can be demuxed by MID. Is that what you were asking? It
wasn't completely clear.

On Mon, Oct 19, 2015 at 11:06 PM, stefan hakansson <notifications@github.com

wrote:

What happens if you create two transceivers (of same type), and two
tracks, and then receive one or two tracks (sort of like SSRCs) in RTP? How
would that be wired up?
Also, #283 #283 had an event fired
when unknown media arrived at any time (i.e. also if there was no ongoing
O/A). That is not supported here - are we fine with removing that? Asking
@martinthomson https://github.com/martinthomson.
Finally: if I were to implement this, I think I would need some more
normative text. An example showing how the Transceiver can be used to
handle early media is good, but the spec must be clear enough to implement.
To be clear: I think we should merge this PR, but knowing there is more
work to be done speccing things out.


Reply to this email directly or view it on GitHub
#293 (comment).

@alvestrand
Copy link
Contributor

Den 20. okt. 2015 20:57, skrev pthatcherg:

I don't remember what the plan for completely unknown media is. I'd have
to go read the f2f minutes/notes to remember. Martin, do you remember?

For media that made it past SRTP verification but can't be associated to
an m-line?
I think it's safe to drop it, but YMMV.

As for normative text, I'll add some in the addTransceiver method saying
that the receiver can receive media before setRemoteDescription is called
with an answer, along with the example which I'll add.

If you create two transceivers of the same type, they'll have different
MIDs, and they can be demuxed by MID. Is that what you were asking? It
wasn't completely clear.

I think that's right. That means MID must be known at addTransceiver
time (or at least before media arrives), which is OK.

On Mon, Oct 19, 2015 at 11:06 PM, stefan hakansson <notifications@github.com

wrote:

What happens if you create two transceivers (of same type), and two
tracks, and then receive one or two tracks (sort of like SSRCs) in RTP? How
would that be wired up?
Also, #283 #283 had an event fired
when unknown media arrived at any time (i.e. also if there was no ongoing
O/A). That is not supported here - are we fine with removing that? Asking
@martinthomson https://github.com/martinthomson.
Finally: if I were to implement this, I think I would need some more
normative text. An example showing how the Transceiver can be used to
handle early media is good, but the spec must be clear enough to implement.
To be clear: I think we should merge this PR, but knowing there is more
work to be done speccing things out.


Reply to this email directly or view it on GitHub
#293 (comment).


Reply to this email directly or view it on GitHub
#293 (comment).

@stefhak
Copy link
Contributor

stefhak commented Oct 21, 2015

Yes, I think this would work. Association is made by the MID field in the RTP header.
This does not however address the case #283 addresses, namely firing an event if there is incoming (authenticated) media "out of the blue" (i.e. no ongoing negotiation, or ongoing negotiation but with a MID that is not part of the outstanding offer). Personally I'm fine with that, just want to raise the question to @martinthomson and @fluffy

@martinthomson
Copy link
Member

As we discussed, I think that this is fine. We can generate messages for the console if we find that diagnosing session errors is difficult.

@stefhak
Copy link
Contributor

stefhak commented Oct 22, 2015

Given that, I propose we merge this PR.

@alvestrand
Copy link
Contributor

Would be great to have a rebase...


<p>If a kind is passed in, the value of the <code>.sender.track</code> will be
null and and media type generated by <code>createOffer</code> will be that of
the kind. The MSID generated by <code>createOffer</code> will be selected by
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the MSID spec, this will cause a track to be created at the remote side after an offer/answer exchange. I assume this is required as part of the "warmup" usage.
Don't we want to prevent this by passing "send:false"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MSID is only generated if needed, such as when "send: true". I will make that more explicit.

@alvestrand
Copy link
Contributor

I reviewed the PR a little more closely than before, and as always, found a few nits. If you can address them while rebasing, that would be great.

@elagerway
Copy link
Contributor

@pthatcherg can you have another look at this, feels like we are close.

@pthatcherg
Copy link
Contributor Author

I'll rebase and address the issues.

On Thu, Oct 22, 2015 at 7:23 AM, Erik Lagerway notifications@github.com
wrote:

@pthatcherg https://github.com/pthatcherg can you have another look at
this, feels like we are close.


Reply to this email directly or view it on GitHub
#293 (comment).

@pthatcherg
Copy link
Contributor Author

I've rebased and addressed the issues.

On Thu, Oct 22, 2015 at 4:41 PM, Peter Thatcher pthatcher@google.com
wrote:

I'll rebase and address the issues.

On Thu, Oct 22, 2015 at 7:23 AM, Erik Lagerway notifications@github.com
wrote:

@pthatcherg https://github.com/pthatcherg can you have another look at
this, feels like we are close.


Reply to this email directly or view it on GitHub
#293 (comment).

@pthatcherg
Copy link
Contributor Author

I also added an example of receiving media before signalling on the offerer
side.

On Fri, Oct 23, 2015 at 4:41 PM, Peter Thatcher pthatcher@google.com
wrote:

I've rebased and addressed the issues.

On Thu, Oct 22, 2015 at 4:41 PM, Peter Thatcher pthatcher@google.com
wrote:

I'll rebase and address the issues.

On Thu, Oct 22, 2015 at 7:23 AM, Erik Lagerway notifications@github.com
wrote:

@pthatcherg https://github.com/pthatcherg can you have another look
at this, feels like we are close.


Reply to this email directly or view it on GitHub
#293 (comment).

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.

How to prevent tracks from being renegotiated after rejection
8 participants