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

Is it illegal to modify sdp between createOffer and setLocalDescription? #2907

Closed
y-i opened this issue Nov 7, 2023 · 7 comments · Fixed by #2928
Closed

Is it illegal to modify sdp between createOffer and setLocalDescription? #2907

y-i opened this issue Nov 7, 2023 · 7 comments · Fixed by #2928
Labels
Needs Test Needs a WPT test

Comments

@y-i
Copy link

y-i commented Nov 7, 2023

I read "4.4.2 Interface Definition" and have a question about [[LastCreatedOffer]].

const offer = await pc.createOffer();
// modify sdp
await pc.setLocalDescription(offer);

Considering the behavior of the above code, according to step 5 of createOffer, the value will be set in [[LastCreatedOffer]] when createOffer is called.

If setLocalDescription is called after editing the SDP, it appears to be rejected with an InvalidModificationError because an sdp different from [[LastCreatedOffer]] is passed in step 4.2.
However, when I actually run it, all modern browsers (Chrome/Firefox/Safari) resolve it.

Am I misunderstanding something about this?
Or do all browsers violate the spec?

@henbos
Copy link
Contributor

henbos commented Nov 7, 2023

All browsers are violating the spec. Modifying SDP is not allowed.

@henbos
Copy link
Contributor

henbos commented Nov 7, 2023

Historically, features have been shipped via SDP modifications rather than APIs. The API side of things have been playing catch-up, e.g. setCodecPreferences() to allow modifying codecs that way instead of "SDP munging" or the recent API to modify header extensions to negotiating in the extension spec. So the list of reasons for SDP munging is quite small these days, but I'm not sure all holes have been plugged just yet.

@henbos
Copy link
Contributor

henbos commented Nov 7, 2023

@jan-ivar @alvestrand Should we make an official list of "holes that needs to be plugged"?

@y-i
Copy link
Author

y-i commented Nov 7, 2023

Thank you for explaining the detail.
In my case, I modified a SDP directly to use L16 to use Lyra codec over WebRTC.

@alvestrand
Copy link
Contributor

alvestrand commented Nov 7, 2023 via email

@henbos henbos added the Needs Test Needs a WPT test label Jan 4, 2024
@henbos
Copy link
Contributor

henbos commented Jan 4, 2024

We should add a test for this (that would fail)

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 17, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 17, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 17, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 18, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 18, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
aarongable pushed a commit to chromium/chromium that referenced this issue Jan 18, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1248758}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 18, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1248758}
fippo added a commit to fippo/webrtc-pc that referenced this issue Jan 18, 2024
@fippo
Copy link
Contributor

fippo commented Jan 18, 2024

Discussion how to enable codecs like L16 should now happen in #2925

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 18, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1248758}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 21, 2024
…nging, a=testonly

Automatic update from web-platform-tests
webrtc wpt: add failing tests for SDP munging

covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1248758}

--

wpt-commits: d38fb52d7074a59d68803b2d211a2a193f78f8bd
wpt-pr: 44044
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Jan 22, 2024
…nging, a=testonly

Automatic update from web-platform-tests
webrtc wpt: add failing tests for SDP munging

covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <phanckemicrosoft.com>
Reviewed-by: Harald Alvestrand <htachromium.org>
Cr-Commit-Position: refs/heads/main{#1248758}

--

wpt-commits: d38fb52d7074a59d68803b2d211a2a193f78f8bd
wpt-pr: 44044

UltraBlame original commit: 90899997ab4060c1cafc0287db868fc8b7db19ef
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Jan 22, 2024
…nging, a=testonly

Automatic update from web-platform-tests
webrtc wpt: add failing tests for SDP munging

covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <phanckemicrosoft.com>
Reviewed-by: Harald Alvestrand <htachromium.org>
Cr-Commit-Position: refs/heads/main{#1248758}

--

wpt-commits: d38fb52d7074a59d68803b2d211a2a193f78f8bd
wpt-pr: 44044

UltraBlame original commit: 90899997ab4060c1cafc0287db868fc8b7db19ef
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 22, 2024
…nging, a=testonly

Automatic update from web-platform-tests
webrtc wpt: add failing tests for SDP munging

covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <phanckemicrosoft.com>
Reviewed-by: Harald Alvestrand <htachromium.org>
Cr-Commit-Position: refs/heads/main{#1248758}

--

wpt-commits: d38fb52d7074a59d68803b2d211a2a193f78f8bd
wpt-pr: 44044

UltraBlame original commit: 90899997ab4060c1cafc0287db868fc8b7db19ef
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Jan 23, 2024
…nging, a=testonly

Automatic update from web-platform-tests
webrtc wpt: add failing tests for SDP munging

covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1248758}

--

wpt-commits: d38fb52d7074a59d68803b2d211a2a193f78f8bd
wpt-pr: 44044
alvestrand pushed a commit that referenced this issue Feb 23, 2024
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Feb 23, 2024
covering both read-only RTCSessionDescription and actual SDP munging.
See also
  w3c/webrtc-pc#2907

BUG=chromium:662898,chromium:823036

Change-Id: Iaabbd131198d18203de4fb7436e4d8c37fb9452e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5204646
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1248758}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Test Needs a WPT test
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants