Add RTP header negotiation control. #25

alvestrand · 2020-01-28T06:03:19Z

Fixes #24

Note: Initial PR has SDP control surface only.

alvestrand · 2020-02-06T13:47:30Z

Now I think the interface is complete (but spec linkages are still not done). Comments welcome!

alvestrand · 2020-02-06T15:55:58Z

Note: WebIDL DOES permit partial dictionary.

alvestrand · 2020-02-12T17:14:43Z

Ping reviewers. It's Thursday tomorrow.
@jan-ivar @henbos

index.html

henbos · 2020-02-13T08:31:26Z

index.html

+              is not present in <var>extlist</var>, throw an IllegalArgument error.
+            </li>
+            <li>
+              Set the [[\HeaderExtensionsToOffer]] to <var>extlist</var>.


If this changes the value of HeaderExtensionsToOffer I think we should fire negotiation needed.

In fact, I think we need to make check if negotiation is needed extendable. Unless we add a placeholder step in webrtc-pc that we can reference and override, this spec could simply say "if check negotiationn needed would have returned false, run the following steps instead:" return true if extensions have been negotiated, false otherwise

I'm OK with resolving negotiation needed in a follow-up PR.

index.html

henbos · 2020-02-13T08:40:14Z

index.html

+      more work on a JSEP revision.
+    </p>
+    <p>
+      In the algorithm to <a>set an SDP description</a>, add a bullet in the sections


This should say "set an RTCSessionDescription" and I think we should clarify that it's a webrtc-pc reference unless the link makes that obvious, considering we just talked about JSEP a bullet point ago.

Good point. It's missing (thus far) in the linked-references list.

Can we add a direct link in the meantime?

henbos · 2020-02-13T08:55:00Z

index.html

+      for processing answer and pranswer (bullet 3.2.8.1.7 and 3.2.8.2.6):
+      <ul>
+        <li>Set the transceiver's [[\HeaderExtensionsNegotiated]] slot to the list of
+          RTP header extensins from the answer.


We need to also say what effects these should have. I.e. start sending and/or receiving according to HeaderExtensionsNegotiated? How does direction come into play here?

There's already a comment in the webrtc spec about which directions apply to sender vs receiver.
We can say the same thing here - the details of when to send various extensions are either in some IETF spec or left to the implementation, so what we can say here is that we enable sending (enabled=true on the sender).
The receiver has no choice about whether to receive or not, but it can choose to act only on those that have been negotiated (discard any others).

index.html

henbos · 2020-02-13T09:02:02Z

index.html

+          <p>
+            When passed to setParameters(), true when the RTP sender should
+            send this header extension when appropriate; false when the RTP
+            sender should never send this extension.


Should this be settable? If the header extension is not negotiated and we pass "true" this needs to throw.

Only negotiated extensions are returned from getParameters(). The URL field is read-only, so we throw if it is changed. Need to say that setParameters() throws if the length of the list is different, too.

henbos · 2020-02-13T09:02:24Z

index.html

+      However, the "uri", "id" and "encrypted" members of
+      RTCRtpHeaderExtensionParameters are now read-only.
+    </p>
+


nit: remove empty line

index.html

jan-ivar

I have questions before I can do a more complete review, but wanted to leave comments in time for the meeting.

jan-ivar · 2020-02-13T14:38:11Z

index.html

+    </p>
+    <p>
+      If, at a later time, it is a good time to add them to the base specification, that
+      will be done. Deciding factors will include maturity of the extension, consensus on


Too much passive language for my taste. I don't think we need to explain. How about just:

This document contains proposed extensions to the [[WEBRTC]] specification. These extensions may be added to the base specification should they reach maturity in implementation and gain consensus.

Since this comment was not addressed yet. Can we bikeshed introduction in a separate issue instead of blocking? For now it's nice to have something even if it can be shortened.

jan-ivar · 2020-02-13T14:43:58Z

index.html

+    </p>
+    <pre class="idl">
+partial dictionary RTCRtpHeaderExtensionCapability {
+  RTCRtpTransceiverDirection direction = "sendrecv";


Needs a dictionary member section and explanation of direction, either here or in a follow up (I don't understand what it does, even from the discussion here).

jan-ivar · 2020-02-13T14:46:15Z

index.html

+};
+    </pre>
+    <p>
+      Let [[\HeaderExtensionsToOffer]] be an internal slot of the RTCRtpTransceiver,


"When creating an RTCRtpTransceiver, add the following steps to the end:"

jan-ivar · 2020-02-13T14:48:18Z

index.html

+      more work on a JSEP revision.
+    </p>
+    <p>
+      In the algorithm to <a>set an SDP description</a>, add a bullet in the sections


Can we add a direct link in the meantime?

jan-ivar · 2020-02-13T14:50:00Z

index.html

+              is not present in <extlist>, throw an IllegalArgument error.
+=======
+              is not present in <var>extlist</var>, throw an IllegalArgument error.
+>>>>>>> 441d821224ddbc98724c985e9639cffe2e20344e


jan-ivar · 2020-02-13T14:51:41Z

index.html

@@ -133,17 +329,17 @@ <h3>
    </p>
    <p>
      Let <code>RTCRtpReceiver</code> objects have a
-      <dfn>[[\PlayoutDelayHint]]</dfn> internal slot initially
+      <dfn>[[\PlayoutDelay]]</dfn> internal slot initially


Unrelated to this PR?

Somehow github is showing diff against the wrong version. That change has been merged to head according to github.io

Junior Oliveira

jan-ivar · 2020-02-13T14:52:32Z

index.html

+      The inclusion of a settable member of RTCRtpHeaderExtensionParameters
+      means that it is no longer a read-only member of RTCRtpParameters.
+      However, the "uri", "id" and "encrypted" members of
+      RTCRtpHeaderExtensionParameters are now read-only.


I'd need more time to grok the overall reasons for this. Is there a short explainer I missed somewhere for what the use case is exactly? That would help me review this further.

This particular clause just notes that the headerExtensions member of RTCRtpParameters used to be read-only, and it is no longer read-only. The sentence needs to be changed to "remain read-only", since the base spec already stated that they were read-only.

henbos · 2020-02-13T14:42:22Z

index.html

+    <p>
+      In the algorithms for generating initial offers in JSEP section 5.1.1,
+      read "for each supported RTP header extension" as "For each RTP header extension
+      listed in [[\HeaderExtensionsToOffer]] where direction is not "stopped"".


The "for each supported RTP header extension" language is in JSEP 5.3.1 (Constructing an Offer - Initial Offers), not 5.1.1.

We need to say how the direction affects the SDP, a spec reference is enough. E.g. "The direction attribute of the a=extmap (RFC5285, Section 6) MUST reflect the direction attribute of the RTCRtpHeaderExtensionCapability".

What about for the answer? You did not modify the paragraph in JSEP 5.3.1 (Generating an Answer - Initial Answers). Is this an offer-only API or is it meant to control what is answered as well? Can I use this API to reject an RTP header extension the same way RTCRtpTransceiverDirection is used in both the offer AND answer in the case of RTCRtpTransceivers? Or will the answer always reply with the maximum direction it is capable of receiving, regardless if the endpoint wants to offer it?

henbos · 2020-02-13T14:50:11Z

index.html

+            </li>
+            <li>
+              Set the [[\HeaderExtensionsToOffer]] to <var>extlist</var>.
+            </li>


"JSEP 5.2.2 Subsequent Offers" says:

In addition, for each existing, non-recycled, non-rejected m= section in the new offer, the following adjustments are made based on the contents of the corresponding m= section in the current local or remote description, as appropriate:
...

The RTP header extensions MUST only include those that are present in the most recent answer.

If we want to override this behavior we must be clear that this is intentional. If we don't want to override this behavior, we should probably throw an exception if you attempt to use this API after [[\HeaderExtensionsNegotiated]] has a value.

henbos · 2020-02-13T14:50:44Z

index.html

+              is not present in <extlist>, throw an IllegalArgument error.
+=======
+              is not present in <var>extlist</var>, throw an IllegalArgument error.
+>>>>>>> 441d821224ddbc98724c985e9639cffe2e20344e


Rebase issue

henbos · 2020-02-13T14:53:15Z

index.html

+      for processing answer and pranswer (bullet 3.2.8.1.7 and 3.2.8.2.6):
+      <ul>
+        <li>Set the transceiver's [[\HeaderExtensionsNegotiated]] slot to the list of
+          RTP header extensins from the answer.


If the transceiver is created because of a remote offer I think we need to set [[\HeaderExtensionsToOffer]] to match the offer, or to say "Set [[\HeaderExtensionsToOffer]] to [[\HeaderExtensionsNegotiated]]" here. Otherwise we will break JSEP rules, which says that any follow-up offers must have the same RTP header extensions as the previous answer.

Actually the way JSEP is written I think HeaderExtensionsToOffer would simply be ignored in follow-up offers if the m= section has previously been negotiated.

Tried a different approach, saying that setting [[HeaderExtensionsToOffer]] can restrict the list of offered extensions, but cannot extend it. That should provide interoperability with implementations that depend on the list never growing (if any).

alvestrand · 2020-02-13T15:41:52Z

Note on RID: The default browser can send rid, but can't receive it. So the negotiation control needs to say that the browser may restrict what directions can be set for specific extensions - not just say "mandatory must be sendrecv".

alvestrand · 2020-02-13T15:47:27Z

Add a comment that getParameters() on the receiver will not return "enabled".

alvestrand · 2020-02-13T15:49:37Z

Should this API only be callable before initial negotiation, or should we modify more JSEP sections (createAnswer, subsequent create*)

henbos · 2020-02-13T15:50:57Z

Please re-upload this PR (looks like merge changes are part of this PR), address comments, and ask for more review.

alvestrand · 2020-02-13T16:05:16Z

@henbos looks as if you're not looking at the 3:55 PM version - I think I got the unresolved diff-mark out of the file in that upload.

detetivepro12 · 2020-02-13T21:55:24Z

index.html

@@ -133,17 +329,17 @@ <h3>
    </p>
    <p>
      Let <code>RTCRtpReceiver</code> objects have a
-      <dfn>[[\PlayoutDelayHint]]</dfn> internal slot initially
+      <dfn>[[\PlayoutDelay]]</dfn> internal slot initially


Junior Oliveira

detetivepro12

alvestrand:rtp-header-control

Fixes w3c#24 Note: Initial PR has SDP control surface only.

alvestrand · 2020-02-14T10:42:51Z

A rebase-and-track dance brought the diff in line with what was expected (no more PlayoutDelayHint in the diffs).

henbos · 2020-02-14T11:15:10Z

Ack, let us know when you want us to take another look

alvestrand · 2020-02-17T15:20:40Z

Got commentary from the guy who's implementing it - leading to some proposed changes:

Expose what's currently set to be sent
That should also expose the default before calling the setter, which allows us to include extensions that aren't on by default as having direction "stopped"
it's probably better to expose setting via a setter method than by modifying the attribute, since the usual tactic is to configure the whole thing in one go rather than piecemeal, and dictionaries are weird wrt setting them (a dictionary is always pass-by-value).

alvestrand · 2020-02-17T16:46:26Z

Please take another look.

henbos · 2020-02-18T14:39:33Z

index.html

+      initialized to the platform-specific list of implemented RTP header extensions.
+      The direction
+      attribute for all extensions that are mandatory to use MUST be initialized to
+      an appropriate value other than "stopped". The direction attribute for extensions


Question: For mandatory extensions, is the "appropriate value" necessarily the same as the direction value present in getCapabilities().headerExtensions[i].direction? Should we have a paragraph that explains how getCapabilities() initializes the direction of its return values?

I'm not clear on whether getCapabilities() should return direction at all; it is able to do so because we used "partial dictionary", but that doesn't mean that we have to.
I think the Principle of Least Astonishment says that sender.getCapabilities() should return the same thing as "addTrancsceiver().headerExtensionsToOffer()", filtered to remove header extensions that can't be set on a sender (if any). Returning differing values seems like a gotcha that we don't need to make.

index.html

henbos · 2020-02-18T14:43:58Z

index.html

+      initialized to an empty list.
+    </p>
+    <p>
+      In the algorithms for generating initial offers in [[RTCWEB-JSEP]] section 5.1.1,


I think this should say section 5.2.1? (https://rtcweb-wg.github.io/jsep/#rfc.section.5.2.1)

Correct. Corrected.

index.html

henbos · 2020-02-18T15:20:53Z

index.html

+    <section>
+      <h2>Modifications to existing procedures</h2>
+      <p>
+        In the <a>final steps to create an offer</a>, add a step:


Do we need to do the same in "final steps to create an answer"? Also is this repeating the same thing as the earlier paragraphs that was referring to JSEP or is this doing something new too?

I deleted this. I think I duplicated these mods by accident.

henbos · 2020-02-18T15:21:15Z

index.html

+      <dl>
+        <dt>enabled, of type boolean</dt>
+        <dd>
+          <p>When returned from getParameters() on an RTPSender, "enabled" is true


RTCRtpSender

henbos · 2020-02-18T15:24:17Z

index.html

+            to false instructs the RTP sender to never send this extension.
+          </p>
+          <p>
+            When calling getParameters() on an RTPReceiver, the "enabled" member will


RTCRtpReceiver

henbos · 2020-02-18T15:25:49Z

index.html

+      <p class="note">
+        The list of extensions returned from getParameters() on a sender will only include
+        the extensions that have been negotiated for sending. Changing the
+        list, apart from setting the "enabled" member, is not permitted.


nit: Maybe make this part of the normative steps? It's testable and it's not clear without the note.

Moved the restriction on changing the list down to where the other effects on RTCRtpParameters are described.

henbos · 2020-02-18T15:29:29Z

index.html

+          </p>
+          <p>
+            When calling getParameters() on an RTPReceiver, the "enabled" member will
+            always be missing, indicating that there is no control surface here.


nit: "indicating that there is no control surface here" is not needed, because RTCRtpReceiver.setParameters() is not a method that exists, so this is true for anything that it returns. Just say it's always missing.

henbos

I think this PR looks mostly good, but:

Since headerExtensionsToOffer is also used to reject extensions in the answer, a better name is headerExtensions.
If direction is also used for answer, we need to say that the answer contains the most relevant direction. Right now it only looks if "stopped" or not.
Need to call out that getCapabilities() does not fill in the direction.
Attempting to make "stopped" extensions in headerExtensionsToOffer non-"stopped" after [[\HeaderExtensionsNegotiated]] has a value should throw an exception. Otherwise this would result in invalid follow-up offers.

henbos · 2020-02-20T13:53:43Z

index.html

+    </p>
+    <p>
+      If, at a later time, it is a good time to add them to the base specification, that
+      will be done. Deciding factors will include maturity of the extension, consensus on


Since this comment was not addressed yet. Can we bikeshed introduction in a separate issue instead of blocking? For now it's nice to have something even if it can be shortened.

henbos · 2020-02-20T13:55:52Z

index.html

+partial interface RTCRtpTransceiver {
+  void setOfferedRtpHeaderExtensions(
+sequence&lt;RTCRtpHeaderExtensionCapability&gt; headerExtensionsToOffer);
+  readonly attribute FrozenArray&lt;RTCRtpHeaderExtensionCapability&gt; headerExtensionsToOffer;


headerExtensionsToOffer sounds offer-specific, but they are also used for the answer, and can be used for rejecting something on offer: Please rename to "headerExtensions"

henbos · 2020-02-20T13:58:26Z

index.html

+        saying "For each transciever, set [[\HeaderExtensionsNegotiated]] to
+        an empty list, and then
+        for each "a=hdrext" line, add an appropriate
+        RTCRtpHeaderExtensionCapability to the list.


nit: Missing ending quotation mark.
nit: "to that list"

henbos · 2020-02-20T13:59:38Z

index.html

+      <h2>Modifications to existing procedures</h2>
+      <p>
+        In the <a>set an RTCSessionDescription</a> algorithm, add a step
+        right after the step that sets transceiver.[[\Sender]].[[\SendCodecs]],


This is actually referring to two separate steps, both relating to setting an answer.

Please clarify that this is referring to two steps and that this only has an effect when setting an answer and going back to stable.

henbos · 2020-02-20T14:07:13Z

index.html

+        In the algorithm for generating initial answers in [[RTCWEB-JSEP]] section 5.3.1, replace "For
+        each supported RTP header extension that is present in the offer" with "For each
+        supported RTP header extension that is present in the offer and is also present in
+        [[\HeaderExtensionsToOffer]] with a direction different from "stopped"".


If the transceiver was created by setting the offer, [[\HeaderExtensionsToOffer]] would have its initial value of whatever the default is. Which means that if the offer contains a non-default header extension that we support, then by default we would reject it.

For transceiver that were created as a result of SRD(offer), might it make more more sense to initialize [[\HeaderExtensionsToOffer]] to the subset of what's on offer that we are capable of accepting?

This has several benefits:

The answerer does not have to figure out which header extensions to add in the most common case of accepting whatever is on offer.

This follows a similar pattern as transceiver.direction/currentDirection.

If between offer and answer, the answerer wants to know what was on offer, it can just check transceiver.headerExtensionsToOffer, because it reflects the offer. No SDP parsing needed.

henbos · 2020-02-20T14:12:05Z

index.html

+        each supported RTP header extension that is present in the offer" with "For each
+        supported RTP header extension that is present in the offer and is also present in
+        [[\HeaderExtensionsToOffer]] with a direction different from "stopped"".
+      </p>


Same comment as in earlier commit: What if what's offered is "sendrecv" but headerExtensionsToOffer is "recvonly"? We should reply with the appropriate direction, not JUST filter out stopped ones.

henbos · 2020-02-20T14:14:42Z

index.html

+      RTP header extensions.
+    </p>
+    <pre class="idl">
+partial dictionary RTCRtpHeaderExtensionCapability {


Can you add a sentence somewhere saying that this attribute is not set for getCapabilities()?

henbos · 2020-02-20T14:19:31Z

index.html

+          </p>
+          <p>
+            When calling getParameters() on an RTCRTPReceiver, the "enabled" member will
+            always be missing.


RTCRtpReceiver

henbos · 2020-02-20T14:19:35Z

index.html

+      <dl>
+        <dt>enabled, of type boolean</dt>
+        <dd>
+          <p>When returned from getParameters() on an RTCRTPSender, "enabled" is true


RTCRtpSender

henbos · 2020-02-20T14:21:45Z

I'm happy to merge and iterate in follow-up PRs if we can all agree on the name of the attribute in this PR. Me arguing for "headerExtensions" rather than "headerExtensionsToOffer".

alvestrand · 2020-02-20T15:38:21Z

index.html

+
+partial interface RTCRtpTransceiver {
+  void setOfferedRtpHeaderExtensions(
+sequence&lt;RTCRtpHeaderExtensionCapability&gt; headerExtensionsToOffer);


Rename to headerExtensionsToNegotiate.

henbos · 2020-02-20T15:40:11Z

Also headerExtensionsToNegotiate should be initialized to what is on offer if the transceiver was created by the offer. A subset of the what's on offer that the endpoint is capable of accepting

alvestrand · 2020-02-21T16:18:20Z

I don't think modifying headerExtensionsToNegotiate on incoming offers is the Right Thing. If so, we need to change the setOfferedRtpHeaderExtensions algorithm to permit-but-ignore URIs that are supported but are not on offer.
We already apply the filtering function in "generate an answer" to only reply with stuff that is also present in the offer; keeping "toNegotiate" as "what I set" makes life simpler.

alvestrand · 2020-02-21T16:24:53Z

@henbos you need to remove your "changes requested" before I can submit.

henbos

Let's merge!

aboba

Since we shipped an equivalent extension in Edge Spartan, we believe that the extension has merit. Note: we found that it was possible for developers to shoot themselves in the foot by incorrectly configuring critical header extensions such as MID. This version of the RTP Header Extension control should make this less likely by limiting the possibility of error. However, the inability to change header extension IDs will also prevent some valid uses (such as the RID/MID simulcast loopback test which currently does SDP munging). This seems like a reasonable tradeoff, but it should be checked via an Origin Trial.

alvestrand requested review from henbos and jan-ivar February 6, 2020 16:05

henbos requested changes Feb 13, 2020

View reviewed changes

jan-ivar requested changes Feb 13, 2020

View reviewed changes

henbos reviewed Feb 13, 2020

View reviewed changes

detetivepro12 approved these changes Feb 13, 2020

View reviewed changes

alvestrand added 5 commits February 14, 2020 11:36

Add RTP header negotiation control.

c59a4a0

Fixes w3c#24 Note: Initial PR has SDP control surface only.

Added the enabled bit

9f5f1c5

Corrected to partial dictionary

aab8121

Another partial

5d39154

Some improvements

5fc2113

alvestrand force-pushed the rtp-header-control branch from 062c476 to 5fc2113 Compare February 14, 2020 10:36

Review comments, add attribute

65d51af

Changed from set-all-at-once to modify-direction

d24d839

henbos requested changes Feb 18, 2020

View reviewed changes

Review comments

bab2ad6

henbos reviewed Feb 20, 2020

View reviewed changes

alvestrand commented Feb 20, 2020

View reviewed changes

henbos added the editors can integrate label Feb 20, 2020

henbos approved these changes Feb 21, 2020

View reviewed changes

henbos merged commit 6961e8f into w3c:master Feb 21, 2020

aboba reviewed Mar 9, 2020

View reviewed changes

alvestrand deleted the rtp-header-control branch May 14, 2020 10:34

Add RTP header negotiation control. #25

Add RTP header negotiation control. #25

Conversation

alvestrand commented Jan 28, 2020

alvestrand commented Feb 6, 2020

alvestrand commented Feb 6, 2020

alvestrand commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvestrand commented Feb 13, 2020

alvestrand commented Feb 13, 2020

alvestrand commented Feb 13, 2020

henbos commented Feb 13, 2020

alvestrand commented Feb 13, 2020

Choose a reason for hiding this comment

detetivepro12 left a comment

Choose a reason for hiding this comment

alvestrand commented Feb 14, 2020

henbos commented Feb 14, 2020

alvestrand commented Feb 17, 2020 • edited Loading

alvestrand commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvestrand Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henbos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henbos commented Feb 20, 2020

Choose a reason for hiding this comment

henbos commented Feb 20, 2020

alvestrand commented Feb 21, 2020

alvestrand commented Feb 21, 2020

henbos left a comment

Choose a reason for hiding this comment

alvestrand commented Feb 17, 2020 •

edited

Loading

alvestrand Feb 20, 2020 •

edited

Loading