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

make setCodecPreferences only look at receive codecs #2926

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Jan 16, 2024

aligning with JSEP:

setCodecPreferences does not directly affect which codec the implementation decides to send.
It only affects which codecs the implementation indicates that it prefers to receive

partial fix for #2888


💥 Error: 502 Bad Gateway 💥

PR Preview failed to build. (Last tried on Feb 1, 2024, 5:14 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL

error code: 502

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@stefhak
Copy link
Contributor

stefhak commented Jan 18, 2024

LGTM.

Copy link
Contributor

@alvestrand alvestrand left a comment

Choose a reason for hiding this comment

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

Discussed in the January 16 WEBRTC WG meeting. I believe this PR is consistent with the outcome of that discussion.

@jan-ivar jan-ivar added the Needs Test Needs a WPT test label Jan 23, 2024
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 31, 2024
tracking w3c/webrtc-pc#2926
* tests now use RTCRtpReceiver.getCapabilities instead of RTCRtpSender
* added test that a sendonly codec throws InvalidModificationError

WebRTC change:
  https://webrtc-review.googlesource.com/c/src/+/328780

BUG=webrtc:15396

Remove obsolete WebRtc-ExposeNonStandardStats feature

which broke when stats were moved to using WebIDL

BUG=chromium:1323230,chromium:1414363

Change-Id: I690b76caf171a8c5ace41badf3b66cd90ffb596d
@fippo
Copy link
Contributor Author

fippo commented Jan 31, 2024

Test added in web-platform-tests/wpt#44318

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 31, 2024
tracking w3c/webrtc-pc#2926
* tests now use RTCRtpReceiver.getCapabilities instead of RTCRtpSender
* added test that a sendonly codec throws InvalidModificationError

WebRTC change:
  https://webrtc-review.googlesource.com/c/src/+/328780

BUG=webrtc:15396

Remove obsolete WebRtc-ExposeNonStandardStats feature

which broke when stats were moved to using WebIDL

BUG=chromium:1323230,chromium:1414363

Change-Id: I690b76caf171a8c5ace41badf3b66cd90ffb596d
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 31, 2024
tracking w3c/webrtc-pc#2926
* tests now use RTCRtpReceiver.getCapabilities instead of RTCRtpSender
* added test that a sendonly codec throws InvalidModificationError

WebRTC change:
  https://webrtc-review.googlesource.com/c/src/+/328780

BUG=webrtc:15396
Change-Id: I690b76caf171a8c5ace41badf3b66cd90ffb596d
@fippo
Copy link
Contributor Author

fippo commented Feb 1, 2024

Amendment worked but validation now throws
"file:/home/runner/work/webrtc-pc/webrtc-pc.w3c/webrtc.html":13397.15-13397.67: error: Element “div” not allowed as child of element “dl” in this context. (Suppressing further errors from this subtree.)
but I am not touching a dl, only a dt? @dontcallmedom you are my only hope!

@dontcallmedom
Copy link
Member

#2910 had the same bogus error, which I didn't quite get to debug; will raise again in my todo list :)

Ensure generation of valid markup
@@ -11181,9 +11181,10 @@ <h2>
<dfn data-idl="">setCodecPreferences</dfn>
</dt>
<dd>
<div id="setcodecpreferences-receive">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I found one more place we forgot to upate. Should we also delete this sentence?

https://w3c.github.io/webrtc-pc/#dfn-final-steps-to-create-an-answer says

If transceiver.direction is "sendonly" or "sendrecv", exclude any codecs not included in the list of implemented send codecs for kind.

Since it should be OK for pc1 to receive X and pc2 to receive Y, we shouldn't filter the preference on send codecs because that would exclude X from pc1's offer if X was a recvonly codec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sentence needs to stay. Until we are ready to more officially deal with answer codecs that are not in the offer which JSEP allows.

webrtc.html Show resolved Hide resolved
Copy link
Contributor

@henbos henbos left a comment

Choose a reason for hiding this comment

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

There be dragons

Copy link
Contributor

@henbos henbos left a comment

Choose a reason for hiding this comment

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

Removing my approval because I think I've identified that this PR breaks sendonly use cases without modifying SDP and considering we already have a transciever.direction filtering in place, I don't see any problem with what the spec says today. See #2932 (comment)

@fippo
Copy link
Contributor Author

fippo commented Feb 8, 2024

Can you give an example of a sendonly codec?

Copy link
Contributor

@henbos henbos left a comment

Choose a reason for hiding this comment

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

Suspecting what I was reacting to was an implementation bug, not a spec issue. Approving again

webrtc.html Show resolved Hide resolved
@alvestrand
Copy link
Contributor

Editors can integrate once tests have landed (and @dontcallmedom has ensured that amendment thingy is right.

@alvestrand alvestrand merged commit 20474d9 into w3c:main Feb 8, 2024
4 checks passed
@fippo fippo deleted the setcodecpreferences-recvonly branch February 8, 2024 15:26
ibaoger pushed a commit to ibaoger/webrtc that referenced this pull request Feb 12, 2024
which is what is noted in JSEP:
  https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences

Some W3C spec modifications are required since the W3C specification
currently takes into account send codecs as well.

Spec issue:
  w3c/webrtc-pc#2888
Spec PR:
 w3c/webrtc-pc#2926

setCodecPreferences continues to modify the codecs in an offer.

Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.

BUG=webrtc:15396

Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
Reviewed-by: Henrik Boström <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#41719}
ibaoger pushed a commit to ibaoger/webrtc that referenced this pull request Feb 13, 2024
This reverts commit 1cce1d7.

Reason for revert: Breaks WPTs

Original change's description:
> Make setCodecPreferences only look at receive codecs
>
> which is what is noted in JSEP:
>   https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences
>
> Some W3C spec modifications are required since the W3C specification
> currently takes into account send codecs as well.
>
> Spec issue:
>   w3c/webrtc-pc#2888
> Spec PR:
>  w3c/webrtc-pc#2926
>
> setCodecPreferences continues to modify the codecs in an offer.
>
> Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.
>
> BUG=webrtc:15396
>
> Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
> Reviewed-by: Henrik Boström <[email protected]>
> Commit-Queue: Philipp Hancke <[email protected]>
> Reviewed-by: Harald Alvestrand <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#41719}

Bug: webrtc:15396
Change-Id: I7b545e91f820c3affc39841c6e93939eac75c363
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339520
Reviewed-by: Harald Alvestrand <[email protected]>
Commit-Queue: Harald Alvestrand <[email protected]>
Owners-Override: Henrik Boström <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Auto-Submit: Henrik Boström <[email protected]>
Bot-Commit: [email protected] <[email protected]>
Cr-Commit-Position: refs/heads/main@{#41725}
@dontcallmedom-bot
Copy link

This issue was mentioned in WEBRTCWG-2024-02-20 (Page 18)

ibaoger pushed a commit to ibaoger/webrtc that referenced this pull request Feb 26, 2024
This is a reland of commit 1cce1d7
after updating the WPT that broke on Mac.

Original change's description:
> Make setCodecPreferences only look at receive codecs
>
> which is what is noted in JSEP:
>   https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences
>
> Some W3C spec modifications are required since the W3C specification
> currently takes into account send codecs as well.
>
> Spec issue:
>   w3c/webrtc-pc#2888
> Spec PR:
>  w3c/webrtc-pc#2926
>
> setCodecPreferences continues to modify the codecs in an offer.
>
> Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.
>
> BUG=webrtc:15396
>
> Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
> Reviewed-by: Henrik Boström <[email protected]>
> Commit-Queue: Philipp Hancke <[email protected]>
> Reviewed-by: Harald Alvestrand <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#41719}

Bug: webrtc:15396
Change-Id: I0c7b17f00de02286f176b500460e17980b83b35b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339541
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#41807}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 15, 2024
Essentially a no-op since we're going to see this change
reverted when we vendor in 1e7a6f3b6a.

Upstream commit: https://webrtc.googlesource.com/src/+/1cce1d7ddcbde3a3648007b5a131bd0c2638724b
    Make setCodecPreferences only look at receive codecs

    which is what is noted in JSEP:
      https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences

    Some W3C spec modifications are required since the W3C specification
    currently takes into account send codecs as well.

    Spec issue:
      w3c/webrtc-pc#2888
    Spec PR:
     w3c/webrtc-pc#2926

    setCodecPreferences continues to modify the codecs in an offer.

    Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.

    BUG=webrtc:15396

    Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
    Reviewed-by: Henrik Boström <[email protected]>
    Commit-Queue: Philipp Hancke <[email protected]>
    Reviewed-by: Harald Alvestrand <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#41719}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 15, 2024
We already cherry-picked this when we vendored 1cce1d7ddc.

Upstream commit: https://webrtc.googlesource.com/src/+/1e7a6f3b6a8eee7efcb129eec10fe734d718ebc8
    Revert "Make setCodecPreferences only look at receive codecs"

    This reverts commit 1cce1d7ddcbde3a3648007b5a131bd0c2638724b.

    Reason for revert: Breaks WPTs

    Original change's description:
    > Make setCodecPreferences only look at receive codecs
    >
    > which is what is noted in JSEP:
    >   https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences
    >
    > Some W3C spec modifications are required since the W3C specification
    > currently takes into account send codecs as well.
    >
    > Spec issue:
    >   w3c/webrtc-pc#2888
    > Spec PR:
    >  w3c/webrtc-pc#2926
    >
    > setCodecPreferences continues to modify the codecs in an offer.
    >
    > Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.
    >
    > BUG=webrtc:15396
    >
    > Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
    > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
    > Reviewed-by: Henrik Boström <[email protected]>
    > Commit-Queue: Philipp Hancke <[email protected]>
    > Reviewed-by: Harald Alvestrand <[email protected]>
    > Cr-Commit-Position: refs/heads/main@{#41719}

    Bug: webrtc:15396
    Change-Id: I7b545e91f820c3affc39841c6e93939eac75c363
    No-Presubmit: true
    No-Tree-Checks: true
    No-Try: true
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339520
    Reviewed-by: Harald Alvestrand <[email protected]>
    Commit-Queue: Harald Alvestrand <[email protected]>
    Owners-Override: Henrik Boström <[email protected]>
    Reviewed-by: Henrik Boström <[email protected]>
    Auto-Submit: Henrik Boström <[email protected]>
    Bot-Commit: [email protected] <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#41725}
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Apr 19, 2024
Essentially a no-op since we're going to see this change
reverted when we vendor in 1e7a6f3b6a.

Upstream commit: https://webrtc.googlesource.com/src/+/1cce1d7ddcbde3a3648007b5a131bd0c2638724b
    Make setCodecPreferences only look at receive codecs

    which is what is noted in JSEP:
      https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences

    Some W3C spec modifications are required since the W3C specification
    currently takes into account send codecs as well.

    Spec issue:
      w3c/webrtc-pc#2888
    Spec PR:
     w3c/webrtc-pc#2926

    setCodecPreferences continues to modify the codecs in an offer.

    Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.

    BUG=webrtc:15396

    Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
    Reviewed-by: Henrik Boström <[email protected]>
    Commit-Queue: Philipp Hancke <[email protected]>
    Reviewed-by: Harald Alvestrand <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#41719}
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Apr 19, 2024
We already cherry-picked this when we vendored 1cce1d7ddc.

Upstream commit: https://webrtc.googlesource.com/src/+/1e7a6f3b6a8eee7efcb129eec10fe734d718ebc8
    Revert "Make setCodecPreferences only look at receive codecs"

    This reverts commit 1cce1d7ddcbde3a3648007b5a131bd0c2638724b.

    Reason for revert: Breaks WPTs

    Original change's description:
    > Make setCodecPreferences only look at receive codecs
    >
    > which is what is noted in JSEP:
    >   https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences
    >
    > Some W3C spec modifications are required since the W3C specification
    > currently takes into account send codecs as well.
    >
    > Spec issue:
    >   w3c/webrtc-pc#2888
    > Spec PR:
    >  w3c/webrtc-pc#2926
    >
    > setCodecPreferences continues to modify the codecs in an offer.
    >
    > Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.
    >
    > BUG=webrtc:15396
    >
    > Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
    > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
    > Reviewed-by: Henrik Boström <[email protected]>
    > Commit-Queue: Philipp Hancke <[email protected]>
    > Reviewed-by: Harald Alvestrand <[email protected]>
    > Cr-Commit-Position: refs/heads/main@{#41719}

    Bug: webrtc:15396
    Change-Id: I7b545e91f820c3affc39841c6e93939eac75c363
    No-Presubmit: true
    No-Tree-Checks: true
    No-Try: true
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339520
    Reviewed-by: Harald Alvestrand <[email protected]>
    Commit-Queue: Harald Alvestrand <[email protected]>
    Owners-Override: Henrik Boström <[email protected]>
    Reviewed-by: Henrik Boström <[email protected]>
    Auto-Submit: Henrik Boström <[email protected]>
    Bot-Commit: [email protected] <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#41725}
daemory pushed a commit to dangxiwang/webrtc that referenced this pull request Apr 19, 2024
This reverts commit 1cce1d7.

Reason for revert: Breaks WPTs

Original change's description:
> Make setCodecPreferences only look at receive codecs
>
> which is what is noted in JSEP:
>   https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences
>
> Some W3C spec modifications are required since the W3C specification
> currently takes into account send codecs as well.
>
> Spec issue:
>   w3c/webrtc-pc#2888
> Spec PR:
>  w3c/webrtc-pc#2926
>
> setCodecPreferences continues to modify the codecs in an offer.
>
> Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.
>
> BUG=webrtc:15396
>
> Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
> Reviewed-by: Henrik Boström <[email protected]>
> Commit-Queue: Philipp Hancke <[email protected]>
> Reviewed-by: Harald Alvestrand <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#41719}

Bug: webrtc:15396
Change-Id: I7b545e91f820c3affc39841c6e93939eac75c363
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 15, 2024
Upstream commit: https://webrtc.googlesource.com/src/+/db2f52ba88cf9f98211df2dabb3f8aca9251c4a2
    Reland "Make setCodecPreferences only look at receive codecs"

    This is a reland of commit 1cce1d7ddcbde3a3648007b5a131bd0c2638724b
    after updating the WPT that broke on Mac.

    Original change's description:
    > Make setCodecPreferences only look at receive codecs
    >
    > which is what is noted in JSEP:
    >   https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences
    >
    > Some W3C spec modifications are required since the W3C specification
    > currently takes into account send codecs as well.
    >
    > Spec issue:
    >   w3c/webrtc-pc#2888
    > Spec PR:
    >  w3c/webrtc-pc#2926
    >
    > setCodecPreferences continues to modify the codecs in an offer.
    >
    > Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.
    >
    > BUG=webrtc:15396
    >
    > Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
    > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
    > Reviewed-by: Henrik Boström <[email protected]>
    > Commit-Queue: Philipp Hancke <[email protected]>
    > Reviewed-by: Harald Alvestrand <[email protected]>
    > Cr-Commit-Position: refs/heads/main@{#41719}

    Bug: webrtc:15396
    Change-Id: I0c7b17f00de02286f176b500460e17980b83b35b
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339541
    Commit-Queue: Philipp Hancke <[email protected]>
    Reviewed-by: Harald Alvestrand <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#41807}
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request May 17, 2024
Upstream commit: https://webrtc.googlesource.com/src/+/db2f52ba88cf9f98211df2dabb3f8aca9251c4a2
    Reland "Make setCodecPreferences only look at receive codecs"

    This is a reland of commit 1cce1d7ddcbde3a3648007b5a131bd0c2638724b
    after updating the WPT that broke on Mac.

    Original change's description:
    > Make setCodecPreferences only look at receive codecs
    >
    > which is what is noted in JSEP:
    >   https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences
    >
    > Some W3C spec modifications are required since the W3C specification
    > currently takes into account send codecs as well.
    >
    > Spec issue:
    >   w3c/webrtc-pc#2888
    > Spec PR:
    >  w3c/webrtc-pc#2926
    >
    > setCodecPreferences continues to modify the codecs in an offer.
    >
    > Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics.
    >
    > BUG=webrtc:15396
    >
    > Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031
    > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780
    > Reviewed-by: Henrik Boström <[email protected]>
    > Commit-Queue: Philipp Hancke <[email protected]>
    > Reviewed-by: Harald Alvestrand <[email protected]>
    > Cr-Commit-Position: refs/heads/main@{#41719}

    Bug: webrtc:15396
    Change-Id: I0c7b17f00de02286f176b500460e17980b83b35b
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339541
    Commit-Queue: Philipp Hancke <[email protected]>
    Reviewed-by: Harald Alvestrand <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#41807}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants