Skip to content

Add remote track channelCount #3025

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.

Sign up for GitHub

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add remote track channelCount #3025

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
308a590
Add remote track channelCount
henbos Nov 26, 2024
9cde211
Merge branch 'main' into 241126_channelCount
henbos Nov 28, 2024
3579baa
Update description
henbos Nov 28, 2024
5d27f27
data-tests
henbos Nov 28, 2024
c4a49af
Merge branch 'main' into 241126_channelCount
dontcallmedom Jan 17, 2025
7475848
Fix amendments.json
dontcallmedom Jan 17, 2025
7eff351
Fix amendments.json
dontcallmedom Jan 17, 2025
20dbf35
Update amendments.json
henbos Mar 27, 2025
61999b9
Address dom comments
henbos Mar 27, 2025
becc7a3
Add MUST language
henbos Mar 27, 2025
57dbd50
nit
henbos Mar 27, 2025
135f52e
Merge branch 'main' into 241126_channelCount
henbos Mar 27, 2025
7f3c5eb
Adapt amendments to appended rows
dontcallmedom Mar 27, 2025
3e03beb
Merge branch 'main' into 241126_channelCount
henbos Mar 28, 2025
577d9cb
Clarify and take stereo flag into account
henbos Mar 28, 2025
ced7c79
Merge branch '241126_channelCount' of https://github.com/henbos/webrt…
henbos Mar 28, 2025
953516d
Local SDP
henbos Mar 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion amendments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,19 @@ function removeComments(el) {

function markInsertion(el, controller) {
const wrapper = document.createElement("ins");
if (el.tagName === "DIV" || el.tagName === "SECTION" || el.tagName === "DT" || el.tagName === "DD" || el.tagName === "LI") {
if (el.tagName === "DIV" || el.tagName === "SECTION" || el.tagName === "DT" || el.tagName === "DD" || el.tagName === "LI" || el.tagName === "TR") {
// special casing the case where <div> is used to group <dt>/<dd>
if (el.tagName === "DIV" && el.parentNode.tagName === "DL") {
for (let child of el.children) {
wrapChildNodes(child, document.createElement("ins"));
}
el.children[0].prepend(controller);
} else if (el.tagName === "TR") {
for (let child of el.children) {
wrapChildNodes(child, document.createElement("ins"));
}
el.children[0].prepend(controller);

} else {
wrapChildNodes(el, wrapper);
el.prepend(controller);
Expand Down
17 changes: 17 additions & 0 deletions amendments.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,23 @@
]
}
],
"def-constraint-channelCount": [
{
"description": "Add channelCount to remote track settings",
"type": "addition",
"status": "candidate",
"difftype": "append",
"id": 53,
"pr": 3025,
"tests": [
"webrtc/RTCRtpEncodingParameters-codec-opus-stereo.https.html"
],
"testUpdates": [
"web-platform-tests/wpt#48599",
"web-platform-tests/wpt#49392"
]
}
],
"codec-match-without-level-id": [
{
"description": "Two codecs are considered the same even if level-id is not",
Expand Down
14 changes: 14 additions & 0 deletions webrtc.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16690,6 +16690,20 @@ <h4>
double rounded to the tenth decimal place.
</td>
</tr>
<tr id="def-constraint-channelCount">
<th data-tests="RTCRtpEncodingParameters-codec-opus-stereo.https.html">
channelCount
</th>
<td>
{{ConstrainULong}}
</td>
<td>
As a setting, this is the number of channels of the latest
audio frame received, reflecting the number of
{{RTCRtpCodec/channels}} that the decoder outputted. A stereo
capable decoder MUST produce stereo and return the value 2.
Copy link

Choose a reason for hiding this comment

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

Define "stereo capable" ?
For opus I interpret the intention to be what RFC 6716 2.1.2 refers to as "stereo decoder". If so this seems fine, since it reflects the data represented by the track. I'm not sure that's obvious from the proposed text though. The bit with "that the decoder outputted" seems clearer, but OTOH seems to contradict "latest audio frame received" in case it doesn't match the decoder configuration.

Copy link
Contributor Author

@henbos henbos Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The intent is that is the browser has implemented opus stereo, then the decoder will output stereo - regardless if the signal on the wire is mono or stereo. You could in theory flip-flop between mono and stereo here, but our audio folks did not want to do this because they didn't see the use case for it and they were concerned that re-initializing the opus decoder in the middle of the stream (the use case is CSRC where the stream can switch between mono and stereo at any time and multiple times throughout the meeting).

Is this OK with you? I can clarify the text to make it more clear, I agree that the latest frame makes it sound like I'm going somewhere else

Copy link
Contributor Author

@henbos henbos Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I.e. I would only expect this to change from 2 to 1 or vice versa if you switch which codec you use, but as long as you use opus you should always get 2 regardless of number of channels on the wire

Copy link

Choose a reason for hiding this comment

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

From what I can tell of the libwebrtc opus decoder (referenced above in #3025 (comment)) it outputs what was signaled. Not sure what you mean by "implements" above but it doesn't sound like you mean "signaled" or "configured"? Are you proposing to return 2 even when negotiating stereo=0?

Copy link
Contributor Author

@henbos henbos Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No, you're right, that nuance got lost on me, my mistake! If we signal stereo=0, we're asking for mono, then I think it makes perfect sense that you get mono, and as per your link that's what we're already doing.

What we want is that is stereo=1 is signalled, then your track has 2 channels regardless if the stream being sent is mono or stereo.

And ultimately we want stereo=1 to be signalled by default because we want you to be able to replaceTrack between mono and stereo tracks on the sender side without having to re-negotiate and without the receiver having to have special logic to deal with this.

Copy link

Choose a reason for hiding this comment

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

I'm not sure about "negotiated". Since the stereo param is receive-only it makes sense to me if the setting matches the stereo param in the local description. Other people know signaling better than me, to say whether those are equivalent for this case.

Otherwise LGTM

Copy link
Contributor Author

@henbos henbos Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

OK - changed from "negotiated" to "when the local SDP was set", which is more precise. (For the infomrative "NOTE" I think the choice of language there doesn't matter)

@jan-ivar @Pehrsons I think this fully captures what we want now and includes that "MUST" language. Can I have the "Editors can integrate" back? :)

Copy link
Member

Choose a reason for hiding this comment

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

After disscussions with @padenot, we've found value in returning channelCount 1 when a stereo capable decoder is receiving mono packets. It's useful because down stream websites can cut processing in half.

I'll comment on the issue in a bit (after updating a fiddle to show this)

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 understand if you want to get it right on the first PR, but considering I'll be OOO for months and this PR currently reflects how libwebrtc-based opus decoders already work today, meaning all browsers that change the default to stereo=1 and exposed channelCount would pass these WPTs, can we leave "channelCount:1 on a stereo=1 decoder" as a separate issue and pull request?

We can already claim consensus on everyone wanting stereo opus by default so it would be good to make progress even if we don't have consensus on "mono on stereo opus" which is a new feature request. @jakobivarsson

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Alternatively we can make the language of this PR less strong, but I'd like to get this off my plate before parental leave and leave the rest to the WG)

</td>
</tr>
</tbody>
</table>
<p class="no-test-needed">
Expand Down