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

RTCRtpParameters.codec matching is probably too strict #2987

Open
docfaraday opened this issue Aug 19, 2024 · 14 comments
Open

RTCRtpParameters.codec matching is probably too strict #2987

docfaraday opened this issue Aug 19, 2024 · 14 comments

Comments

@docfaraday
Copy link
Contributor

Let's say this is one of the codecs returned by getCapabilities, and JS sets a sender's RTCRtpParameters.codec to it before negotiation.

{ mimeType: 'video/vp8', clockRate: 90000, sdpFmtpLine: 'max-fs=12288;max-fr=60' }

Then, suppose the remote side's fmtp for vp8 looks like this, because it only wants to receive 30 frames per second (instead of the default of 60):

a=fmtp:120 max-fs=12288;max-fr=30

This results in the following entry in RTCRtpParameters.codecs:

{ mimeType: 'video/vp8', clockRate: 90000, sdpFmtpLine: 'max-fs=12288;max-fr=30', payloadType: 120 }

The codec dictionary match algorithm says these do not match, despite the fact that the negotiation worked just fine for vp8. Most fmtp parameters are negotiable in this way. I get the impression that a failure to match here is not the intent.

@docfaraday
Copy link
Contributor Author

docfaraday commented Aug 19, 2024

I should note that semantic matching of sdpFmtpLine would not be sufficient to fix this problem; max-fr=60 and max-fr=30 are not semantically equivalent, but we probably still want the .codec setting to work here. If we want to be able to match these, we'd basically have to specify that the browser consider it a match if the same codec/channels/clockRate/fmtp in a remote description would be considered a successful negotiation of that codec.

Alternately, we could punt and say that .codec is only ever matched against RTCRtpParameters.codecs (ie; negotiated codecs with parameters from a remote description), and that setting .codec based on getCapabilities, or before negotiation, is simply not allowed.

@docfaraday
Copy link
Contributor Author

Fundamentally, the problem we have here is that most fmtp values are not a distinguishing feature of a codec, but in rare cases they are (eg; profile-level-id for H264). Right now, the spec treats all fmtp as distinguishing features, to make sure the tiny minority of distinguishing ones are taken into account.

@aboba
Copy link
Contributor

aboba commented Aug 19, 2024

The problem is an assumption that symmetry is required, when many fmtp parameters refer to a receiver capability that need not be the same for both peers. For example, in VP8 one peer can have max-fr=30 and the other can have max-fr=60. Even profiles need not match for H.265 and AV1 and even for H.264 as long as level-asymmetry-allowed=1. Maybe we need to specify what fmtp parameters actually need to match?

@fippo
Copy link
Contributor

fippo commented Aug 19, 2024

.codec must be taken (literal match) from the remote because the remote specifies the parameters it wants to receive, no?

@docfaraday
Copy link
Contributor Author

docfaraday commented Aug 19, 2024

It depends I suppose; if JS sets VP8 with 60 fps as its preferred codec, and the other end says it wants to do H264, or VP8 with 30fps, should we say "Oh well" and just do H264? If we're going to require that level of granularity for a match, then maybe semantic matching would be sufficient (lexical still won't work, because "max-fs=12288;max-fr=60" won't match "max-fr=60;max-fs=12288", which is clearly wrong).

@fippo
Copy link
Contributor

fippo commented Aug 19, 2024

But you can only take the input to .codec from the remote capabilities so there must be a lexical match against that.
And then you need a "sdp match" against what you can send. H264 profile-level is the main example here.

Didn't we discuss this in #2888 (without conclusion)? In particular this but it is more about sCP, not setParameters. Same problem though.

@docfaraday
Copy link
Contributor Author

docfaraday commented Aug 19, 2024

Right now the spec allows (and wpt tests for) populating .codec from the return of getCapabilities.

(If we disallow this, that would solve this problem, probably. You could still in theory run into unexpected behavior if the remote side changes its fmtp without changing the semantics, I guess.)

@alvestrand
Copy link
Contributor

I'm currently working in this area (coming in from PT assignment, but "are these equal" comes in a lot there).
Codec capabilities are what one wants to receive, and the PT assignment in the two directions is, in theory, independent, so that this:
a -> b: rtpmap:97 vp8; max-fr=60
b->a: rtpmap:97 vp8; max-fr=30

means that A can receive vp8 at 60 fps, and B can receive vp8 at 30 fps, and they just happen to use the same PT number.
But the idea of "negotiating" codecs kind of doesn't match with that.

@jan-ivar
Copy link
Member

jan-ivar commented Aug 23, 2024

Note the codec dictionary match algorithm only exists because codecs aren't interfaces. See #2955 (comment). It exists to differentiate codecs — i.e. these two are not the same thing, vs these two are the same thing — it does NOT speak to what codecs UAs should select in any particular situation (except where explicitly called from algorithms).

Had they been interfaces, it would clear up that the only allowed API inputs are local API outputs. E.g. constructing your own dictionary based on remote information is invalid if not found locally.

That said, I don't have a strong opinion on how browsers should respond when a codec parameter becomes outdated. Best effort seems desirable given there's little in the way of reading back the effect of a set codec parameter, except stats.

@docfaraday
Copy link
Contributor Author

I'm currently working in this area (coming in from PT assignment, but "are these equal" comes in a lot there). Codec capabilities are what one wants to receive, and the PT assignment in the two directions is, in theory, independent, so that this: a -> b: rtpmap:97 vp8; max-fr=60 b->a: rtpmap:97 vp8; max-fr=30

means that A can receive vp8 at 60 fps, and B can receive vp8 at 30 fps, and they just happen to use the same PT number. But the idea of "negotiating" codecs kind of doesn't match with that.

Sure, but .codec is explicitly about what we should send; it will be matched against whatever is in the remote SDP. If we set .codec based on a RTCRtpSender.getCapabilities call (which the spec explicitly allows for), and the fmtp in the capabilities differs even slightly from what the remote puts in its SDP, we won't try to honor that .codec setting because it doesn't match. That's the issue here.

@jan-ivar
Copy link
Member

jan-ivar commented Aug 23, 2024

... fmtp in the capabilities differs even slightly from what the remote puts in its SDP, we won't try to honor that .codec setting because it doesn't match.

I don't believe the spec says what to do here. The codec dictionary algorithm is only used to validate JS inputs AFAICT.

All it says about it seems inferred here: "Optional value selecting which codec is used for this encoding's RTP stream. If absent, the user agent can chose to use any negotiated codec."

IOW, there's no RTP stream "selecting codec" algorithm.

The purpose seems to be to choose between "negotiated codecs", i.e. parameters.codec exists in parameters.codecs as an invariant. So for people who prefer strict APIs it is tempting to limit inputs to getParameters().codecs. But I suppose even that set may change after renegotiation, so we'd still need to clarify what to do when that invariant breaks.

@docfaraday
Copy link
Contributor Author

docfaraday commented Aug 23, 2024

... fmtp in the capabilities differs even slightly from what the remote puts in its SDP, we won't try to honor that .codec setting because it doesn't match.

I don't believe the spec says what to do here. The codec dictionary algorithm is only used to validate JS inputs AFAICT.

All it says about it seems inferred here: "Optional value selecting which codec is used for this encoding's RTP stream. If absent, the user agent can chose to use any negotiated codec."

Well, that's a separate (and easier) bug. Right now, the wpt expects setting .codec based on getCapabilities, and then negotiating, will cause that codec to be used if it matches the remote SDP. There's no explicit prose around doing this in the spec right now, but it definitely seems to be the intent. The wpt also expects .codec to be automatically unset if the remote SDP does not contain a match (there's definitely nothing in the spec about this).

Also, there's the choosableCodecs stuff in setParameters now that has .codec being compared to:

  1. The negotiated codecs, if set
  2. The codecs configured with setCodecPreferences, if set
  3. The codecs in getCapabilities, if all else fails

This strongly suggests that it is "normal" to be setting .codec based on getCapabilities, which I suppose is why we see the wpt using this pattern.

@Orphis
Copy link
Contributor

Orphis commented Aug 27, 2024

The wpt also expects .codec to be automatically unset if the remote SDP does not contain a match (there's definitely nothing in the spec about this).

The wording is in: https://w3c.github.io/webrtc-pc/#set-the-session-description 4.6.13.2

For each transceiver in connection's set of transceivers:

  1. Let codecs be transceiver.[[Sender]].[[SendCodecs]].
  2. If codecs is not an empty list:
    Remove any codec value in transceiver.[[Sender]].[[SendEncodings]] that does not match any entry in codecs.

We can update it if it's not clear enough.

@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

No branches or pull requests

7 participants