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

setCodecPreferences vs unidirectional codecs #2888

Closed
fippo opened this issue Aug 11, 2023 · 26 comments
Closed

setCodecPreferences vs unidirectional codecs #2888

fippo opened this issue Aug 11, 2023 · 26 comments

Comments

@fippo
Copy link
Contributor

fippo commented Aug 11, 2023

(for after TPAC, this is a minor detail and we have more important things to discuss)

https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-setcodecpreferences
Step 6 says that

If the intersection between codecs and RTCRtpSender.getCapabilities(kind).codecs or the intersection between codecs and
RTCRtpReceiver.getCapabilities(kind).codecs only contains RTX, RED or FEC codecs or is an empty set, throw InvalidModificationError.
This ensures that we always have something to offer, regardless of transceiver.direction.

This means that you must negotiate at least one codec that you can send and receive.

While this seems harmless, it causes trouble with asymmetric codec support and that is a fairly common thing. E.g. in Chrome on Windows we have 14 send codecs and 21 receive codecs, with the asymmetric ones ranging from H264 over VP9 to AV1.
The worst example we found was Android where the H264 send and receive profiles are not an exact string match and we have support for sending 42e032, 4d0032 and 640032 while being able to receive 42001f, 4d001f and 64001f. This lead to errors in a fairly innocent attempt to restrict the codecs to H264 variants:

const codecs = RTCRtpSender.getCapabilities('codecs')
  .filter(c => c.mimeType.toLowerCase() === 'video/H264');
someTransceiver.setCodecPreferences(codecs);

The sentence "this ensures that we always have something to offer" is there to avoid the subsequent problem, not being able to negotiate a codec when the transceiver direction changes like this:

t.direction = 'recvonly'
t.setCodecPreferences({a-codec-you-can-only-encode})

If this was taken to createOffer and setLocalDescription it might lead to an m= line that is rejected which would cause the transceiver to be stopped which would be quite surprising.

setCodecPreferences might check the direction in that case but that just leads to the following scenario:

t.setCodecPreferences({a-codec-you-can-only-encode})
t.direction = 'recvonly'

One approach to solving this is that an attempt to set the direction to 'recvonly' might throw. This also requires a compability check in setCodecPreferences in case the direction was set before setCodecPreferences.

The other alternative would be to let createOffer (which knows about send+recv codes as well as the direction) throw an error when it detects this mismatch. It gets a bit finicky to figure out which transceiver was causing this, the mid might not be available.

Overall I think this is an edge case but the first example is a fairly common flow that is currently broken (depending on the codec support)

Also: how is the "intersection of codecs" defined? It seems that for H264 this must take into account whether level-asymmetry is allowed, no?

@henbos
Copy link
Contributor

henbos commented Aug 11, 2023

I like failing fast. I.e. I'd rather have setCodecPreferences or direction throw than createOffer.

So I think setCodecPreferences with a sendonly codec but the transciever is recvonly should throw an exception.
Likewise if you've configured sendonly codecs with a sendonly transceiver and you attempt to set the direction to recvonly, setting that attribute will throw the exception (yes setters can throw, we already do this e.g. if the transceiver is stopped).

What if you want to change from sendonly codec+transceiver to recvonly codec+transceiver? It's a bit of an edge case, but you can still do that and avoid exception, you just have to make sure to set the direction to inactive as a middle step to prevent throwing.

@steely-glint
Copy link

steely-glint commented Aug 11, 2023

Do the h264 'mismatches' actually fail to decode?
Our (limited) experience is that if you simply lie and claim to be sending constrained baseline and actually send something else (eg high) it decodes just fine. Whereas if you tell the truth about what you want to send the negotiation fails.

So I think perhaps the correct 'fix' here is relaxing the matching rules.

@alvestrand
Copy link
Contributor

@steely-glint this is probably a different bug - that browsers use a library codec without actually investigating what it's capable of decoding, or how to represent that as a profile-level. (Chrome, for instance, uses ffmpeg for decode, which is capable of decoding a lot of stuff, but uses openh264 for encode, which is only capable of encoding constrained baseline. I don't think we ever tell ffmpeg what promises we've made on its behalf.)

@alvestrand
Copy link
Contributor

setCodecPreferences() is setting the list of PTs at the end of the m= line, which indicates which codecs you prefer to receive.

Send-only codecs should either be silently dropped from the list (and an empty list should be an error), or they should cause the setCodecPreferences() call to fail.

@fippo
Copy link
Contributor Author

fippo commented Nov 21, 2023

Harald:

setCodecPreferences() is setting the list of PTs at the end of the m= line, which indicates which codecs you prefer to receive.

The spec says this:

setCodecPreferences will reject attempts to set codecs not matching codecs found in RTCRtpSender.getCapabilities(kind) or
RTCRtpReceiver.getCapabilities(kind), where kind is the kind of the RTCRtpTransceiver on which the method is called.

and a little below

If the intersection between codecs and RTCRtpSender.getCapabilities(kind).codecs

which led me to believe that the input also comes from the sender capabilities.

JSEP makes it quite clear what the original intent was:

Note that 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

but webrtc-pc does not? The solution seems to be to align with JSEP.

@henbos
Copy link
Contributor

henbos commented Nov 22, 2023

The order in the codec list is purely a receive preference that can be ignored.

But the set of codecs - which ones are present in the negotiation - does affect what is allowed to be sent and/or received. You can't send something that doesn't have a PT or which the other side does not recognize.

The sentence that says If the intersection between codecs... is not intended to go against JSEP, I remember adding it as a protection against shooting yourself in the foot where one of the directions has an empty set of codecs. But if we want to support unidirectional codecs, which I think we do, then this sentence should be deleted or modified.

You can always get around it by adding another codec to the list as a dummy codec ensuring that there always is something that could be used for sending or receiving, but that is a bad API design when we want to support uni-direction.

Fixing this would not be a backwards compat issue since it would only expand the set of possibilities beyond what they are today.

@henbos
Copy link
Contributor

henbos commented Nov 22, 2023

Negotiating that you can receive something that you don't know how to receive though is a problem. JSEP doesn't seem to support that which would lead to dropping packets? (It's a problem, but it is a "soft" problem not a "hard" problem)

@fippo
Copy link
Contributor Author

fippo commented Dec 4, 2023

Trying to summarize the current state of discussion (from this code change).

JSEP is pretty clear:

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

So sCP (setCodecPreferences ) is not something you use to pick the send codec.
This works just because send codec is dictated by receive preference in SDP semantics

sCP should not take send codecs into account which webrtc-pc does here.

Answer may include codecs not offered, see https://www.rfc-editor.org/rfc/rfc8829.html#section-5.3.1

"Any currently available media formats that are not present in the current remote description MUST be added after all existing formats"

No browsers do that, but behave as intended (or can be tricked?)

sCP should typically be done in ontrack, see updated samples PR, e.g.
e.transceiver.setCodecPreferences instead of addTransceiver + setCodecPreferences. This often gets nicer in terms of architecture

This hopefully is a somewhat web-compatible change. Woes with unidirectional codecs were not discovered for years
It will still be surprising for developers...

The proposed fix is threefold:

  1. Fix webrtc-pc by removing mentions of send codecs in setCodecPreferences. preview here
  2. Clarify “codecs match” algorithm which says

“If either (but not both) of first.sdpFmtpLine and second.sdpFmtpLine are missing, or if they both exist and first.sdpFmtpLine is different from second.sdpFmtpLine, return false.”

  1. drive-by: the spec is missing consideration for Comfort Noise (oddball, not a media codec, not a resiliency mechanism)

Open question: what does setCodecPreferences do for a sendonly m-line?
Current behavior: only negotiate receive codecs for symmetry

@henbos
Copy link
Contributor

henbos commented Dec 4, 2023

I support making setCodecPreferences only control receiver side preferences, which shouldn't be controversial considering JSEP says they are receiver preferences. More motivation based on the JSEP rule about order below.

"Any currently available media formats that are not present in the current remote description MUST be added after all existing formats"

This JSEP quote has implications. If I want to send H264 but receive VP8 I can do this:

  1. No need to signal H264 in the offer since I prefer to receive VP8, so setCodecPreferences(VP8). But now I depend on the other endpoint adding H264. It can either add H264 and keep VP8 as a backup, but if so JSEP says it should do "VP8, H264" which implies it prefers VP8 even though it wants to prefer H264. It could either break this JSEP rule and do "H264, VP8" anyway (which would work but it seems strange to depend on endpoints breaking JSEP rules as the official way to do something) or it could pretend not to support VP8 and remove it, such that it can respond only with "H264". The latter option would work, but now we've prevented the other endpoint from dynamically switching to VP8, which might not be what we want to do either.
  2. Another option is to offer setCodecPreferences(VP8, H264) in order to allow the other endpoint to make H264 first in the response, while still correctly signaling that we prefer to receive VP8 before H264. And this works today where all codecs have sendrecv capabilities, but how would this work if H264 was instead a sendonly codec that we don't know how to receive? What does it mean to prefer to receive something we don't know how to decode? Using the API this way seems to break down in a world of truly unidirectional codec capabilities.

Based on 2) breaking down, I think 1) is the only way to go, i.e. SDP says this is receiver preferences so let's treat it as such. Send codecs are irrelevant and don't need to be on the list.

So going with 1), how would we deal with a JSEP-respecting endpoint that responds "VP8, H264" instead of "H264, VP8"? I think the answer is @Orphis' setParameters(codec:H264) which allows sending a codec regardless of its placement on the list, as long as it was negotiated.

Open question: what does setCodecPreferences do for a sendonly m-line?

Not much, it sounds like. But what does JSEP say, is it allowed to have an empty list of codecs in the SDP or are we forced to add dummy codecs? The current API rule that says "treat empty list as using browser defaults" seems to not work here... backwards-compat issues incoming?

@henbos
Copy link
Contributor

henbos commented Dec 4, 2023

But it might still be controversial if there are backwards compat issues... setCodecPreferences() currently controls the m= line, regardless of direction or if offerer or answerer, so I don't fully understand the implications.

E.g. if we get an offer that says "VP8, H264" and because we want to send VP8 (respect the offer) but only want to receive H264, so we setCodecPreferneces(H264) before the answer and then complete the negotiation... will our sender still be configured to do VP8? Or does the fact that I removed it for our answer also remove it from my sender configurations? This might not work today...

@fippo
Copy link
Contributor Author

fippo commented Dec 4, 2023

I suspect a lot of the more complicated scenarios are talking to non-WebRTC endpoints. And I hope that most of those use only sendonly/recvonly transceivers and not sendrecv.

https://jsfiddle.net/fippo/n8upqxbL/1/ shows how sCP does handle asymmetry today which seems surprising but is ok?

@alvestrand
Copy link
Contributor

Unfortunately I fear that a majority of the non-webrtc (legacy) endpoints are using only sendrecv.....

@henbos
Copy link
Contributor

henbos commented Dec 4, 2023

@fippo's fiddle gives me hope though, it shows that we support sending one codec and receiving another, and it does so using only setCodecPreferences (no SDP munging).

What is "weird" is that setCodecPreferences also affects what is on offer, but I think that is fine, because it is always the answerer's responsibility to update the preferences to affect what they want to receive. Really the offer is just a suggestion (weirdly enough).

However I think we still have one problem, if I set pc1 to only include VP8 in Fippo's fiddle, then the fiddle breaks: https://jsfiddle.net/henbos/snwcm35e/

I suspect the "bug" here is that we only include codecs that are in both offer and answer? Both send and receive? But if we take JSEP seriously, then there should be no problem to offer ONLY VP8 and answer ONLY VP9. (It breaks even before reaching the follow up offer in the other direction which is what I wanted to test originally)

@alvestrand
Copy link
Contributor

My (current) thinking is that the list of codecs in a=rtpmap lines is the list of codecs we want to have a PT assigned for - no matter if they're receiving or sending.

The list set in setCodecPreferences() is the list that we want to receive. By implication, anything that's in the a=rtpmap lines and not in the setCodecPreferences()-generated list is something the responder should treat as a send-only codec.

At the moment there's no API to control the generation of a=rtpmap lines. If setCodecPreferences() affects that, then we're holding it wrong. (I think.)
We need new API if we want to reduce the number of offered codecs. Or we need SDP munging - it only matters in setRemoteDescription() anyway.

@henbos
Copy link
Contributor

henbos commented Dec 4, 2023

That's clever (having a=rtpmap contain all codecs you support, but the preferences could be a subset of the PTs). This lets the answering endpoint prefer to receive the codec in any order it likes, even if the offerer does not prefer to receive it at all.

We still have the issue that the SDP answerer has no way of knowing the directional support of a codec that is listed in the a=rtpmap line, but it sounds like this is a limitation of SDP rather than a limitation of webrtc-pc APIs.

So I think we can live with it: the app can easily check codec directionality support using RTCRtpSender/Receiver.getCapabilities and communicate those to the other endpoint if it so desires, but that would not be covered by SDP. If the app prefers to receive something that the other endpoint does not support for sending, well then that endpoint will simply send with another codec in the preference list, or if there is no other codec in the preference list, simply not send at all (which is already a situation you can end up in today if codec supports are not overlapping).

@alvestrand
Copy link
Contributor

At a higher level of abstraction, I think we should add a "direction" attribute to the codec description - with a default value of "sendrecv" for backwards compatibility. This would expose the possible usages of the codec to JS in an unambiguous way. Mapping this to representation in SDP is still unclear, but at least we would have the information to build on.

@stefhak
Copy link
Contributor

stefhak commented Jan 9, 2024

Is this related?: I'm looking at a case where the offerer offers sendonly video. I struggle on how to enable the receiving application to apply codec preferences (re-order and prune). I think you can setParameters (including codec data) on an RTPSender, but is there any similar for an RTPReceiver, or how should this be done?

@Orphis
Copy link
Contributor

Orphis commented Jan 9, 2024

Is this related?: I'm looking at a case where the offerer offers sendonly video. I struggle on how to enable the receiving application to apply codec preferences (re-order and prune). I think you can setParameters (including codec data) on an RTPSender, but is there any similar for an RTPReceiver, or how should this be done?

if your receive side is the answerer, you can apply your codec preferences in between the setLocalDescription() call and createAnswer().

@stefhak
Copy link
Contributor

stefhak commented Jan 9, 2024

Thanks @Orphis. I guess that is what I want to do, but I struggle on how to apply those preferences.

Edit, it should be between the setRemoteDescription (not Local) with incoming sendonly offer and createAnswer, right? Anyway, it is not clear to me how to do that.

@fippo
Copy link
Contributor Author

fippo commented Jan 10, 2024

Dumping what I originally had in the slides which are now a bit more condensed:

  • We (apart from Harald) did not read JSEP carefully enough
    • “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”
    • sCP is not something you use to pick the send codec.
    • This works just because send codec is dictated by receive preference in SDP semantics
    • sCP should not take send codecs into account which webrtc-pc does
  • Answer may include codecs not offered
  • Typically done in ontrack, see updated samples PR
    • e.transceiver.setCodecPreferences instead of addTransceiver+setCodecPreferences
  • Somewhat web-compatible change
    • Woes with unidirectional codecs were not discovered for years
    • It will still be surprising for developers
  • Fix webrtc-pc by removing mentions of send codecs in setCodecPreferences
  • Clarify “codecs match” algorithm
    • “If either (but not both) of first.sdpFmtpLine and second.sdpFmtpLine are missing, or if they both exist and first.sdpFmtpLine is different from second.sdpFmtpLine, return false.”
    • H264 profile level-asymmetry-allowed needs to be taken into account
    • Inference of default values might need to be handled as well
  • Drive-by: fix missing consideration for Comfort Noise
    • Oddball, not a media codec, not a resiliency mechanism
  • Open question: what does setCodecPreferences do for a sendonly m-line?
    • Current behavior: only negotiate receive codecs for symmetry. Ok?

@stefhak you may find the samples PR useful. It seemed awkward at first but is actually easier to work with since you have one ontrack handler but many ways to add a transceiver

@stefhak
Copy link
Contributor

stefhak commented Jan 10, 2024

@fippo thanks for a good summary!

For my case (unidirectional medai), things now make sense as the receiving (also being SDP answerer) end can use transceiver.setCodecPreferences to indicate preferred receive codec(s). Exactly what I was looking for.

One slightly confusing thing for me is that webrtc-extensions adds the possibility to define a codec with setParameters on the RTCRTPsender. Can't this be a problem if used before negotiation? In that case any of the implemented (send) codecs can be used, so the promise probably resolves, but the receiver may not support it, which only becomes apparent after SDP o/a.

@henbos
Copy link
Contributor

henbos commented Jan 10, 2024

There's also (from RFC3264):

For streams marked as recvonly in the answer, the "m=" line MUST
contain at least one media format the answerer is willing to receive
with from amongst those listed in the offer. The stream MAY indicate
additional media formats, not listed in the corresponding stream in
the offer, that the answerer is willing to receive.
[...] For streams marked as sendrecv in the answer,
the "m=" line MUST contain at least one codec the answerer is willing
to both send and receive, from amongst those listed in the offer.

So while we can add codecs we want to receive that were not present in the offer, it appears that what is offered cannot be entirely distinct either, as at least one codec needs to either be present in the offer or that is sendrecv-capable?

@Orphis
Copy link
Contributor

Orphis commented Jan 10, 2024

One slightly confusing thing for me is that webrtc-extensions adds the possibility to define a codec with setParameters on the RTCRTPsender. Can't this be a problem if used before negotiation? In that case any of the implemented (send) codecs can be used, so the promise probably resolves, but the receiver may not support it, which only becomes apparent after SDP o/a.

Section 7.2.3 handles that, and if the codec is not in the SendCodecs list, the forced codec setting will be removed and it'll default to the browser's choice. It should still be valid with the proposed fixes.

@aboba
Copy link
Contributor

aboba commented Feb 10, 2024

@fippo
Copy link
Contributor Author

fippo commented Feb 11, 2024

Clarify “codecs match” algorithm
“If either (but not both) of first.sdpFmtpLine and second.sdpFmtpLine are missing, or if they both exist
and first.sdpFmtpLine is different from second.sdpFmtpLine, return false.”

This one turned out to be interesting. For setCodecPreferences this needs to be an exact match, i.e. input needs to come from getCapabilities. Also things like level-asymetry-allowed must not be taken into account.
No normative spec changes are required but I'll add some WPT.

ibaoger pushed a commit to ibaoger/webrtc that referenced this issue 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 issue 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}
ibaoger pushed a commit to ibaoger/webrtc that referenced this issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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}
@henbos
Copy link
Contributor

henbos commented Oct 2, 2024

Closing this issue in favor of the more recent #3006 that reflects what we discussed at the latest TPAC.

However while we settled on the path forward (Proposal A of the slides) we didn't discuss how to handle errors (e.g. changing direction and not having any codecs applicable to that direction anymore) so we'll have to continue at next meeting

@henbos henbos closed this as completed Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants