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

Add RTCRtpEncodingParameters.codec to change the active codec #147

Merged
merged 2 commits into from Apr 20, 2023

Conversation

Orphis
Copy link
Contributor

@Orphis Orphis commented Mar 16, 2023

This new encoding parameter allows changing the active codec on any simulcast layer as long as it is supported or negotiated.

It allows user agent that don't support the feature partially or fully to reject the configuration change.

Fixes #43 , #126


Preview | Diff

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
different codec values on the different encodings, return a promise [= rejected =]
with a newly [= exception/created =] {{OperationError}}.
</p>
</li>
Copy link
Collaborator

@henbos henbos Mar 16, 2023

Choose a reason for hiding this comment

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

In parallel, reconfigure the encoder? Edit: maybe superflous, we already have language "In parallel, configure the media stack to use parameters to transmit sender.[[SenderTrack]]."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the UA should be able to determine ahead of time if it is able to apply this type of configuration, so it should be able to reject it early, without a parallel step. setParameters() does have a provision for handling other errors with an OperationError which would cover late discovery too.

Though addTransceiver() does not have such error handling. What are we supposed to do if we run out of HW resources when using it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Luckily addTransceiver() will not acquire any resources, because it hasn't been negotiated yet. The spec currently says to acquire during negotiation, but as pointed out by Bernard in the last Virtual interim meeting, in practise we don't acquire encoder resources until when we actually use them, so any failure would have to be surfaced then - this is an existing problem, and perhaps we'll want to event handlers for this in the future, but that's a separate issue

Copy link
Member

Choose a reason for hiding this comment

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

I think the UA should be able to determine ahead of time if it is able to apply this type of configuration, so it should be able to reject it early, without a parallel step.

Depends on your definition of "apply". UAs may be able to reject some combinations early, but it's not clear to me that success is guaranteed any more strongly for customized codecs than it is for default ones.

In any case, while rejecting a promise early has some benefit while stepping through in a JS debugger, there's not much else to gain from that particular timing.

@henbos
Copy link
Collaborator

henbos commented Mar 16, 2023

Looks great, it just needs some clarifications

@Orphis
Copy link
Contributor Author

Orphis commented Mar 21, 2023

Note: the current version still lacks fallback behaviour on negotiation (the requested codec is negotiated away and the codec field should be cleared).

@Orphis
Copy link
Contributor Author

Orphis commented Mar 21, 2023

@dontcallmedom Could you help me figure out why {{RTCRtpTransceiver/[[PreferredCodecs]]}} is not working?

@dontcallmedom
Copy link
Member

the problem is that the definition of {{RTCRtpTransceiver/[[PreferredCodecs]]}} in webrtc-pc is marked as not exported - I'll look into fixing it

Copy link
Collaborator

@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.

LGTM % one minor clarification

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
@@ -555,7 +556,102 @@ <h2>Dictionary {{RTCRtpEncodingParameters}} Members</h2>
allows the <a>user agent</a> to adapt its bandwidth allocation strategy
based on the current network conditions.</p>
</dd>
<dt><dfn data-idl>codec</dfn> of type <span class="idlMemberType">RTCRtpCodec</span></dt>
<dd>
<p>Optional value overriding the codec used for this encoding. The {{RTCRtpCodec}} dictionary is
Copy link
Member

Choose a reason for hiding this comment

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

The slides presented justified the ask for simulcast, which is video only. So I think we have agreement for this API for video only.

Suggested change
<p>Optional value overriding the codec used for this encoding. The {{RTCRtpCodec}} dictionary is
<p>This member can only be present if the sender's <code class=
"gum">kind</code> is `"video"`.
Optional value overriding the codec used for this encoding. The {{RTCRtpCodec}} dictionary is

We should make sure this rejects for audio if present on input, like we do for the other video-specific things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The "negotiation once, never re-negotiate" use case is becoming more popular, where one uses CSRCs to identify which participant is which, but always having a fixed number of transceivers.

When you can do negotiationless codec switching for video, I think it is only a matter of time before there will be an ask for audio as well.

That said the audio use case seems a lot more "at risk" than the video use case. For now I don't mind limiting scope to video-only, but I don't think we should rule out the possibility long term.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the bullet points on this slide is "make it possible to change codec without re-negotiating". But this API is much more important for video than for audio due to the other two bullet points which are both video-only uses:

  • Allow different codecs on different encodings.
  • Allow specifying both codec and scalabilityMode with a single API call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should restrict this API to video only. Codec is a general concept, why should it only apply for video only?
While the slides were presented with examples centered around video, with the emergence of new audio codecs, it seems even more important to keep the symmetry in the APIs.

Copy link
Contributor

@fippo fippo Mar 31, 2023

Choose a reason for hiding this comment

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

The slides presented justified the ask for simulcast, which is video only. So I think we have agreement for this API for video only.

Simulcast as a concept is becoming important for audio too, e.g. for cases that want to do PSTN gateway for some participants without transcoding which might mean sending Opus and G722 (or G711) in the same packet.

This API has video use-cases without simulcast too, e.g. you might want to switch codecs because the far end now needs you to downgrade from a modern codec to the video-equivalent of G711 for reasons.

I do not see a reason to artificially restrict the specification.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
different codec values on the different encodings, return a promise [= rejected =]
with a newly [= exception/created =] {{OperationError}}.
</p>
</li>
Copy link
Member

Choose a reason for hiding this comment

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

I think the UA should be able to determine ahead of time if it is able to apply this type of configuration, so it should be able to reject it early, without a parallel step.

Depends on your definition of "apply". UAs may be able to reject some combinations early, but it's not clear to me that success is guaranteed any more strongly for customized codecs than it is for default ones.

In any case, while rejecting a promise early has some benefit while stepping through in a JS debugger, there's not much else to gain from that particular timing.

index.html Outdated Show resolved Hide resolved
index.html Outdated
Comment on lines 629 to 630
<p>If the user agent does not support changing the codec for any encoding or mixing
different codec values on the different encodings,
Copy link
Member

Choose a reason for hiding this comment

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

"mixing different codec values" seems a bit vague. What exactly are the requirements for not throwing OperationError?

Copy link
Contributor

Choose a reason for hiding this comment

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

In practice, does this just mean "doesn't support the codec attribute"? Do existing browsers throw an OperationError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OperationError is already part of the specification when setParameters() cannot allocate resources. Chrome is able to throw those when unknown errors occur deep in the pipeline, which we associate with a resource allocation issue for now.
This describes a new behaviour though and I believe returning an OperationError in case of a complex unsupported input might be relevant.

@dontcallmedom-bot
Copy link

dontcallmedom-bot commented Mar 29, 2023

@w3c w3c deleted a comment from dontcallmedom-bot Mar 29, 2023
Copy link
Contributor

@aboba aboba left a comment

Choose a reason for hiding this comment

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

The checks for setParameters() seem to allow for setting of combinations of codec and scalabilityMode that are illegal (e.g. setting VP8 and L3T3). To fix this, I think we need to add a check in WebRTC-SVC.

index.html Outdated
Comment on lines 579 to 580
<p>Add the following steps in the parameter validation for {{RTCRtpSender/setParameters()}},
before any validation of <var>scalabilityMode</var> from [[WEBRTC-SVC]]:</p>
Copy link
Member

Choose a reason for hiding this comment

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

This adds an unnecessary normative reference to [[WEBRTC_SVC]], which we can avoid. I also struggled with where exactly it meant, and readers shouldn't have to look up a third spec to find out. scalebilityMode also seems irrelevant. Can we express it differently? Same for addTransceiver below.

Looking up https://w3c.github.io/webrtc-svc/#setparameters it says: "[WEBRTC] Section 5.2 describes validation of parameters within setParameters(). Add the following to the conditions under which the operation causes a promise rejected with an InvalidModificationError (step 4 within step 6):"

That doesn't seem to be the right place. Where exactly is this to be inserted?

I've added w3c/webrtc-pc#2854 to try to help.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that not that the normative reference is incorrect - the problem is that the combined procedure doesn't produce the results you'd expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal there is to make sure that the scalabilityMode value is checked against the capabilities associated with codec and not against the general capabilities, codec preferences or first codec in the negotiated list as mentioned in WEBRTC-SVC. Any guidance to do this correctly would be appreciated.

index.html Outdated
<p>Add the following steps in the parameter validation for {{RTCRtpSender/setParameters()}},
before any validation of <var>scalabilityMode</var> from [[WEBRTC-SVC]]:</p>
<ol>
<li><p>Let <var>codecs</var> be <var> parameters</var>.{{RTCRtpParameters/codecs}}.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, webrtc-pc already contains this (unused!) line, so assuming w3c/webrtc-pc#2854 gets merged, how about something like: "Under the [=setParameters validation steps=]", replace this line:

  1. Let codecs be parameters.codecs.

with:

  1. Let choosableCodecs be parameters.codecs.
  2. If choosableCodecs is an empty list, set choosableCodecs to transceiver.{{RTCRtpTransceiver/[[PreferredCodecs]]}}
  3. If choosableCodecs is still an empty list, set choosableCodecs to the [=list of implemented send codecs=] for [=transceiver kind=]

And then follow SVC's lead above and say: "Add the following to the conditions under which the operation causes a promise rejected with an InvalidModificationError (step 4 within step 6):"

  • Any encoding in encodings [=map/exists|contains=] a codec [= codec match | not found =] in choosableCodecs

This should avoid the if/else/else and hopefully be readable. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

To precent attempts to set illegal combinations of codecs and scalabilityMode values, I think you need to also set the "most preferred codec" slot and refer to this slot in WebRTC-SVC Section 4.2.2.

Copy link
Contributor

@aboba aboba left a comment

Choose a reason for hiding this comment

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

I don't think that the proposed checks work together with the WebRTC-SVC checks to produce the expected results. See other comments.

Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

whoops, had this pending comment from before that was not submitted

index.html Show resolved Hide resolved
@dontcallmedom-bot
Copy link

This issue had an associated resolution in WebRTC WG 2023-04-18 – (PR 147: Add RTCRtpEncodingParameters.codec to change the active codec (Florent)):

RESOLUTION: No objection, let’s include audio.

This new encoding parameter allows changing the active codec on any
simulcast layer as long as it is supported or negotiated.
It allows user agent that don't support the feature partially or fully
to reject the configuration change.

Fixes w3c#43 , w3c#126
@Orphis Orphis requested review from aboba and jan-ivar April 20, 2023 13:43
index.html Outdated Show resolved Hide resolved
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
@aboba aboba merged commit 1ae522b into w3c:main Apr 20, 2023
2 checks passed
@dontcallmedom-bot
Copy link

This issue was mentioned in WEBRTCWG-2023-06-27 (Page 46)

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

Successfully merging this pull request may close these issues.

Negotiationless codec selection and per-encoding layer codec configuration Mixed-codec simulcast proposal
8 participants