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

Should setOfferedHeaderExtensions be allowed to restart stopped extensions? #134

Open
alvestrand opened this issue Dec 4, 2022 · 21 comments

Comments

@alvestrand
Copy link
Collaborator

Spinoff from #132 since we should decide what makes sense.

JSEP section 5.2.2 says that subsequent offer/answer can only use the set of header extensions from the initial offer/answer - no adding header extensions later.

We modify (in section 6.1) the procedures for subsequent offers indicating that it is OK to turn on header extensions later.
If this is needed in a realistic scenario, we should support it; if it's not needed, there's no real reason for changing JSEP on this point.

Possible approaches are:

  1. Allow turning on header extensions later (overriding current JSEP text)
  2. Let SetOfferedHeaderExtensions throw an InvalidStateError if the negotiation state is not "new" and the call attempts to change an extensions that is not in [[HeaderExtensionsNegotiated]] from "stopped" to something else
  3. Let the algorithm in section 6.1 iterate over [[HeaderExtensionsNegotiated]] rather than over [[HeaderExtensionsToOffer]], effectively following the current JSEP rule, but providing no feedback to the user that he attempted something impossible

My current thinking is that I think favor approach #2, possibly also doing #3. I'm sure there are other possible approaches.

@fippo
Copy link
Contributor

fippo commented Dec 4, 2022

How does this work currently for setCodecPreferences?
From JSEP 5.2.2 wrt media formats:

and MUST include all currently available formats

but indeed for header extensions:

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

One of the arguments I made for not needing support for enabled was that you can just renegotiate any header extensions. As far as I can see,
https://jsfiddle.net/fippo/6waoyLqs/1/
reoffers ssrc-audio-level in Chrome and Firefox.

Same for codecs where browsers also do not do what JSEP says:
https://jsfiddle.net/fippo/6waoyLqs/2/
(Firefox behaves slightly different here, re-adding opus at the end while Chrome just puts it first again.)

@fippo
Copy link
Contributor

fippo commented Dec 5, 2022

IMO we should specify what browsers currently support, try to negotiate header extensions again and let O/A figure it out

@fippo
Copy link
Contributor

fippo commented Jan 19, 2023

Note that we already do modify JSEP here

@henbos
Copy link
Collaborator

henbos commented Mar 2, 2023

The spec already says...

In the algorithm for generating subsequent offers in [RTCWEB-JSEP] section 5.2.2, replace "The RTP header extensions MUST only include those that are present in the most recent answer" with "For each RTP header extension listed in [[HeaderExtensionsToNegotiate]], and where direction is not "stopped", generate an appropriate "a=extmap" line with "direction" set according to the rules of [RFC5285] section 6, considering the direction in [[HeaderExtensionsToNegotiate]] to indicate the answerer's desired usage".

So it appears we are already allowing restarting stopped extensions. If we are OK with that, we can close this issue as WontFix. Otherwise if we want to be consistent with the recent changes to the API then we should just restrict the direction to what is possible rather than to throw, but I'm not sure I see any benefit to not being allowed to turn something on just because it was previously missing so I think I'd rather just close this issue... thoughts?

@henbos
Copy link
Collaborator

henbos commented Mar 2, 2023

@youennf To answer your question about what is the difference between "inactive" and "stopped", this issue may be a hint towards why there could be a difference between removing and just making it inactive

@henbos
Copy link
Collaborator

henbos commented Mar 2, 2023

According to @fippo the current implementation in Chrome already supports turning extensions on after the fact

@fippo
Copy link
Contributor

fippo commented Mar 2, 2023

The problem is not the text here but that what JSEP describes is not what browsers do currently, even ones that do not implement this extension, see above.

Further, we say in 6.1 step 2.1 that non-negotiated extensions should be set to stopped. This means they will not, unless we modify the other steps, be reoffered by default. The solution here seems to be to set non-negotiated ones to inactive so that they will show up in subsequent offers unless a user of this API takes action.

@henbos
Copy link
Collaborator

henbos commented Mar 3, 2023

I guess there is still value in making an extension "stopped" (removing it from the SDP) versus just "inactive" in the sense that you're not burning unused IDs for the extensions in the RTP packets which may affect whether or not the extended IDs are needed.

As the spec is currently (after recent changes), we'll try re-offering whatever the [[HeaderExtensionsToNegotiate]] are every time. Just like transceiver.direction is always offered even if transciever.currentDirection gets downgraded every time

@henbos
Copy link
Collaborator

henbos commented Mar 3, 2023

Once an ID has been burned though I assume we never recycle it?

@fippo
Copy link
Contributor

fippo commented Mar 3, 2023

You can't change ids (in theory, Harald can tell you the true story some time) during the lifetime of a session.

As the spec is currently (after recent changes), we'll try re-offering whatever the [[HeaderExtensionsToNegotiate]] are every time.

I still don't see that. The handling of remote descriptions in the "set a session description" modifications from 6.1 makes anything not negotiated inactive which means that the modified "algorithm for generating subsequent offers" you quoted will not include them.

Lets circle back after looking at the code?

@henbos
Copy link
Collaborator

henbos commented Mar 3, 2023

In the algorithm for generating subsequent offers in [RTCWEB-JSEP] section 5.2.2, replace "The RTP header extensions MUST only include those that are present in the most recent answer" with "For each RTP header extension listed in [[HeaderExtensionsToNegotiate]], ...

I read this as "we put [[HeaderExtensionsToNegotiate]] in the SDP offer every time".
It may be the case that they get rejected every time ([[NegotiatedHeaderExtensions]] being a subset of that), but that doesn't stop us trying? Or is there anything we're referencing that contradicts this?

@henbos
Copy link
Collaborator

henbos commented Mar 9, 2023

Proposal: Yes it should + close this issue?

@fippo
Copy link
Contributor

fippo commented Mar 9, 2023

The spec is written to override JSEP so the answer (not the proposal) is already yes?
My last comment turned out to be wrong from looking at the code.

@ekr
Copy link

ekr commented Apr 20, 2023

Without taking a position on this technical change, it's not appropriate for W3C specs to override JSEP.

The correct thing to do is to make the change in JSEP-bis, currently in the RFC Editor Queue
https://datatracker.ietf.org/doc/draft-uberti-rtcweb-rfc8829bis/history/

@docfaraday
Copy link

Without taking a position on this technical change, it's not appropriate for W3C specs to override JSEP.

The correct thing to do is to make the change in JSEP-bis, currently in the RFC Editor Queue https://datatracker.ietf.org/doc/draft-uberti-rtcweb-rfc8829bis/history/

Agreed.

@henbos
Copy link
Collaborator

henbos commented Apr 21, 2023

Without taking a position on this technical change, it's not appropriate for W3C specs to override JSEP.

https://w3c.github.io/webrtc-extensions/#rtp-header-extension-control-modifications already has a note about this: "Since JSEP does not know about WebRTC internal slots, merging this change requires more work on a JSEP revision."

I believe this refers to what would be needed to merge this into webrtc-pc.

@fippo
Copy link
Contributor

fippo commented Apr 21, 2023

https://jsfiddle.net/fippo/6waoyLqs/1/ shows that browsers already ignore JSEP when it comes to renegotiation of header extension and this does not require SDP munging (in the "change between createOffer and SLD sense)

@ekr
Copy link

ekr commented Apr 21, 2023

If browsers ignore JSEP, then either we should change JSEP to match or the browsers should be made to match. But just patching this in WebRTC-PC doesn't solve the problem. @juberti, thoughts?

@aboba
Copy link
Contributor

aboba commented Apr 21, 2023

I don't think it is reasonable to expect JSEP-bis to change to reference internal slots. Also, the proposed changes to the text don't result in a parsable sentence.

@aboba
Copy link
Contributor

aboba commented May 14, 2023

@dontcallmedom-bot
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants