Support track level stereo and red setting #470

cnderrauber · 2022-10-14T07:33:58Z

No description provided.

changeset-bot · 2022-10-14T07:34:01Z

🦋 Changeset detected

Latest commit: bdeff9c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
livekit-client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

cnderrauber · 2022-10-14T07:36:13Z

Fix livekit/livekit#970

boks1971 · 2022-10-14T07:37:45Z

src/room/PCTransport.ts

+
+      media.fmtp.some((fmtp): boolean => {
+        if (fmtp.payload === opusPayload) {
+          if (fmtp.config.includes('sprop-stereo=1')) {


Does server have to send this?

yeah, it should have it if sending a stereo track

boks1971 · 2022-10-14T07:39:37Z

src/room/participant/LocalParticipant.ts

+                publishOptions.red = false;
+              }
+              if (publishOptions.dtx !== true) {
+                publishOptions.dtx = false;


This and above is already checking for not true and setting to false. Should the check be if set to true, override it with false when stereo?

the red and dtx can be left to undefined, means use default values. So this only disable them if dtx and red not enabled explicitly

should this logic maybe live in publishTrack instead of setTrackEnabled ?
If users choose to use the publishTrack API directly, I guess they would have to handle all this by themselves otherwise?

yeah I agree this should be in publishTrack.

why do we need to set dtx or red to default false? It seems that we shouldn't change the default behavior of other unrelated params.

also, the logic would be easier to read with the ?? operator

publishOptions ??= {}; publishOptions.red ??= false publishOptions.dtx ??= true

@lukasIO sounds good, will move it there.
@davidzhao For stereo audio tracks, it will use high bitrates encoding (128kbps or higher) and most scenario is music, RED will cost more bandwidth and DTX might cause noise in special case or device. So disable them by default in stereo, user can enable them manually if need

solved by 9b8a0a4

boks1971 · 2022-10-14T07:42:14Z

src/room/PCTransport.ts

+    if (stereoMids.includes(media.mid!)) {
+      media.fmtp.some((fmtp): boolean => {
+        if (fmtp.payload === opusPayload) {
+          fmtp.config += ';stereo=1';


Should this check if config already has stereo=1 and add only if not there? i. e. will multiple pass through here keep adding stereo=1? Guess it probably does not affect functionality, but the SDP could look strange.

Test it will not be added repeatedly in chrome, but the check is safe for various browsers, will add it

davidzhao · 2022-10-15T00:45:45Z

src/room/participant/LocalParticipant.ts

+                publishOptions.red = false;
+              }
+              if (publishOptions.dtx !== true) {
+                publishOptions.dtx = false;


yeah I agree this should be in publishTrack.

why do we need to set dtx or red to default false? It seems that we shouldn't change the default behavior of other unrelated params.

also, the logic would be easier to read with the ?? operator

publishOptions ??= {}; publishOptions.red ??= false publishOptions.dtx ??= true

davidzhao · 2022-10-15T00:53:10Z

src/room/participant/LocalParticipant.ts

+          // disable red and dtx for stereo track if not enabled explicitly
+          let channelCount: ConstrainULong | undefined;
+          if (options) {
+            if ('channelCount' in options) {


when this is moved to publishTrack, determine the actual channelCount will be a challenge. we could take in a stereo?: boolean parameter in publishOptions too, to give user control in case we cannot detect correctly.

In Safari, we could get channelCount from MediaStreamTrack.getSettings(). but Chrome doesn't seem to support that.. which means we may have to go look for it in the constraints.

it's also not fully fail proof but chrome should support at least getConstraints() that would return the channel count in case it was manually set, which we could use as an indication of the user to publish stereo.

* Move all logic into and set explicit defaults * remove unnecessary null check * fix check * only log warning when red or dtx aren't set

davidzhao

looks good! remaining question about defaults

davidzhao · 2022-10-17T19:01:34Z

src/room/participant/LocalParticipant.ts

+          `Opus RED and DTX will be disabled for stereo tracks by default. Enable them explicitly to make it work.`,
+        );
+      }
+      options.red ??= false;


would be great if we don't have to do this. It's confusing for the end user if what we indicate as defaults could change due to another parameter.

red: I think the additional bw requirements is ok. If users doesn't want the redundancy, they could turn it off by themselves

dtx: do you think this would cause issues when used in conjunction? can we test to validate?

I haven't had the chance to catch up on this whole thread, but I'm excited to test it out later this week!

Regarding red support, I will say that from my testing - high bitrate stereo + red (with DTX, noise canceling, echo canceling, etc. all turned off), audio sounds AMAZING. Using similar bitrates with opus by itself doesn't sound nearly as good. For my "quality audio mode" I plan to leave red enabled.

thanks for the feedback @bekriebel ! that's wonderful to hear it made a significant difference in audio quality.

It is hard to reproduce, but a few users report noise with special music cases before, and we close DTX to fix it. At least, opus decoder will generate comfort noise when received dtx packet, that is no needed and might harm to quality in music case. So I suggest disable dtx in stereo mode

davidzhao

lg!

src/room/participant/LocalParticipant.ts

Co-authored-by: David Zhao <dz@livekit.io>

* Support track level stereo and red setting * check stereo is already exist * move options calc to publishTrack * prettier * Move all logic into `publishTrack` and set explicit defaults (livekit#474) * Move all logic into and set explicit defaults * remove unnecessary null check * fix check * only log warning when red or dtx aren't set * not disable red in stereo by default * Update src/room/participant/LocalParticipant.ts Co-authored-by: David Zhao <dz@livekit.io> Co-authored-by: lukasIO <mail@lukasseiler.de> Co-authored-by: David Zhao <dz@livekit.io>

Support track level stereo and red setting

5fccefb

cnderrauber requested review from davidzhao, boks1971 and lukasIO October 14, 2022 07:33

boks1971 reviewed Oct 14, 2022

View reviewed changes

check stereo is already exist

a909779

boks1971 approved these changes Oct 14, 2022

View reviewed changes

davidzhao requested changes Oct 15, 2022

View reviewed changes

cnderrauber added 2 commits October 17, 2022 10:47

move options calc to publishTrack

198c92f

prettier

9b8a0a4

cnderrauber requested a review from davidzhao October 17, 2022 07:55

Move all logic into publishTrack and set explicit defaults (#474)

7542aeb

* Move all logic into and set explicit defaults * remove unnecessary null check * fix check * only log warning when red or dtx aren't set

lukasIO approved these changes Oct 17, 2022

View reviewed changes

davidzhao reviewed Oct 17, 2022

View reviewed changes

not disable red in stereo by default

542b439

davidzhao approved these changes Oct 18, 2022

View reviewed changes

src/room/participant/LocalParticipant.ts Outdated Show resolved Hide resolved

Update src/room/participant/LocalParticipant.ts

bdeff9c

Co-authored-by: David Zhao <dz@livekit.io>

cnderrauber merged commit 0c0e5cc into main Oct 18, 2022

cnderrauber deleted the stereo branch October 18, 2022 08:22

github-actions bot mentioned this pull request Oct 18, 2022

Version Packages #473

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support track level stereo and red setting #470

Support track level stereo and red setting #470

cnderrauber commented Oct 14, 2022

changeset-bot bot commented Oct 14, 2022 •

edited

Loading

cnderrauber commented Oct 14, 2022

boks1971 Oct 14, 2022

cnderrauber Oct 14, 2022

boks1971 Oct 14, 2022

cnderrauber Oct 14, 2022

lukasIO Oct 14, 2022

davidzhao Oct 15, 2022

cnderrauber Oct 17, 2022 •

edited

Loading

cnderrauber Oct 17, 2022

boks1971 Oct 14, 2022

cnderrauber Oct 14, 2022

davidzhao Oct 15, 2022

davidzhao Oct 15, 2022

lukasIO Oct 15, 2022 •

edited

Loading

davidzhao left a comment

davidzhao Oct 17, 2022

bekriebel Oct 17, 2022

davidzhao Oct 17, 2022

cnderrauber Oct 18, 2022

davidzhao left a comment

Support track level stereo and red setting #470

Support track level stereo and red setting #470

Conversation

cnderrauber commented Oct 14, 2022

changeset-bot bot commented Oct 14, 2022 • edited Loading

🦋 Changeset detected

cnderrauber commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnderrauber Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasIO Oct 15, 2022 • edited Loading

Choose a reason for hiding this comment

davidzhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidzhao left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Oct 14, 2022 •

edited

Loading

cnderrauber Oct 17, 2022 •

edited

Loading

lukasIO Oct 15, 2022 •

edited

Loading