Support sending and receiving redundant audio data to help reduce the effects of packet loss on audio quality. #2745

pracheth · 2023-08-31T19:34:22Z

Issue #:

Description of changes:

Support sending and receiving redundant audio data to help reduce the effects of packet loss on audio quality. This will be enabled by default on chromium and webkit based browsers. Firefox unfortunately does not support this feature due to browser API limitations. README also provides instructions to disable feature if needed.

Testing:

Dev testing and unit testing
QA testing.

Can these tested using a demo application? Please provide reproducible step-by-step instructions.

Yes but you will also need a network link conditioner (NLC) on macOS to trigger redundant audio as it is sent only once packet loss is detected and is above a certain threshold. You can set NLC to 30% in either direction and speak. Subjectively speaking, there should be no cut-outs in audio or robotic voice. Note that redundant audio packets are only sent for speech/music. If there is no voice activity detected, no redundant packets will be sent.

Checklist:

Have you successfully run npm run build:release locally?
Y
Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
Yes this has been reviewed internally. Please reach out for the API doc.
Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
N

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

hensmi-amazon

Just some flyby nits, will continue taking a look later!

README.md

hensmi-amazon · 2023-08-31T19:48:32Z

demos/browser/app/meetingV2/meetingV2.html

@@ -156,6 +156,10 @@ <h5 class="modal-title" id="additional-options-modal-label">Additional Options</
            <label for="echo-reduction-capability" class="form-check-label">Use Echo Reduction (new meetings
              only)</label>
          </div>
+          <div class="form-check" id="disable-audio-redundancy-checkbox" style="text-align: left; display: block;">


Is disabling RED meant to be common? Do we need demo coverage of it disabled? Asking because having it in the demo kinda encourages builders to disable it. We don't want a builder to disable RED just because one end-user had a disconnection or something.

Not sure about common but it is something a builder can do. I did put in a note in the readme that we encourage this be turned on and a builder can turn this off only if their customers have very strict bandwidth limitations.

What do you mean by demo coverage? You mean adding it to the canary?

It is probably adding it to demo for now as long as it is not the default behavior in case we need to reproduce case w/ or w/o it later.

re: trung, this is the default behavior. Understandable if we want to reproduce issues, though i wonder how frequent that would be where we need to build in UX to disable it (as opposed to just hacking in the change manually). Always a tough call with the kitchen sink demo lol.

Yeah we have 2 separate needs: one kitchen sink with every knobs to test and one streamline for demo. Whenever we have time we would need to separate these out :D

hensmi-amazon · 2023-08-31T19:51:06Z

demos/browser/app/meetingV2/meetingV2.ts

@@ -1635,6 +1680,7 @@ export class DemoMeetingApp

  metricsDidReceive(clientMetricReport: ClientMetricReport): void {
    this.logAudioStreamPPS(clientMetricReport);
+    this.logRedRecoveryPercent(clientMetricReport);


Nit: Would be nice to encapsulate this and logAudioStreamPps in a function/class outside of this monolithic file.

src/audioprofile/AudioProfile.ts

test/videouplinkbandwidthpolicy/NScaleVideoUplinkBandwidthPolicy.test.ts

src/transceivercontroller/TransceiverController.ts

test/audiovideocontroller/DefaultAudioVideoController.test.ts

demos/browser/app/meetingV2/meetingV2.ts

ltrung · 2023-09-05T21:35:50Z

README.md

+depending on the amount of packet loss detected. The SDK will automatically stop sending redundant data if it hasn't
+detected any packet loss for 5 minutes.
+
+This feature is not supported on Firefox at the moment.


So it works for Chrome, Safari, and iOS and Android?

Yeah. Basically chromium and webkit browsers

Technically we also support Opera. So maybe we should be more specific. Also will all browser version works or only a certain version?

I added some extra information. In the latest push, I also added fallback to peer connection without RED if we determine that we cannot enable it. Tested this on an older version of safari on saucelabs and was able to send/receive audio/video fine.

ltrung · 2023-09-06T02:55:18Z

README.md

+meetingSession.audioVideo.setContentAudioProfile(AudioProfile.fullbandSpeechMono(false));
+```
+
+While there is an option to disable the feature, we recommend keeping it enabled for improved audio quality.


Does CSP have to have worker directive for this to work?

I believe it is already there:

amazon-chime-sdk-js/demos/browser/webpack.config.js

Line 29 in b29a0cb

'worker-src': "'self' blob:",

We have tested this by deploying the demo to the SDK dev account and did not see any csp related issues. Is there any other place that needs update?

I also added a line in the new README section just to call it out that blob is needed. Also verified that the policy we recommend in https://aws.github.io/amazon-chime-sdk-js/modules/contentsecurity_policy.html already has this.

src/transceivercontroller/DefaultTransceiverController.ts

ltrung · 2023-09-06T03:07:53Z

src/transceivercontroller/DefaultTransceiverController.ts

+      })
+    );
+    this.logger.info(`[AudioRed] Redundant audio worker URL ${this.audioRedWorkerURL}`);
+    this.audioRedWorker = new Worker(this.audioRedWorkerURL);


Can this fail? Should we catch error here?

I guess it can theoretically fail. Lemme force the audioRedWorker to null and see behavior.

Looks like this could fail for reasons mentioned in https://developer.mozilla.org/en-US/docs/Web/API/Worker/Worker

Updated to account for failure and restart connection with redundancy disabled so that user connects to the meeting. Tested this out by removing blob from csp worker-src and connecting to a meeting. Tested both audio and video sending and receiving. Also added unit tests.

mklingb

LGTM!

… effects of packet loss on audio quality.

pracheth requested a review from a team as a code owner August 31, 2023 19:34

pracheth requested review from ltrung, devalevenkatesh, simmkyu and mklingb August 31, 2023 19:34

hensmi-amazon reviewed Aug 31, 2023

View reviewed changes

pracheth force-pushed the audio-redundancy branch 2 times, most recently from 276f101 to 0f6795c Compare September 5, 2023 17:50

dinmin-amzn reviewed Sep 5, 2023

View reviewed changes

test/audiovideocontroller/DefaultAudioVideoController.test.ts Outdated Show resolved Hide resolved

demos/browser/app/meetingV2/meetingV2.ts Outdated Show resolved Hide resolved

ltrung reviewed Sep 5, 2023

View reviewed changes

ltrung reviewed Sep 6, 2023

View reviewed changes

src/transceivercontroller/DefaultTransceiverController.ts Outdated Show resolved Hide resolved

ltrung reviewed Sep 6, 2023

View reviewed changes

pracheth force-pushed the audio-redundancy branch 3 times, most recently from 5f0de01 to a791d39 Compare September 11, 2023 20:45

dinmin-amzn approved these changes Sep 11, 2023

View reviewed changes

pracheth force-pushed the audio-redundancy branch from a791d39 to 70c34b4 Compare September 12, 2023 16:44

pracheth requested review from dinmin-amzn, ltrung and hensmi-amazon September 12, 2023 16:52

mklingb approved these changes Sep 12, 2023

View reviewed changes

dinmin-amzn approved these changes Sep 12, 2023

View reviewed changes

Support sending and receiving redundant audio data to help reduce the…

6b5c8d6

… effects of packet loss on audio quality.

pracheth force-pushed the audio-redundancy branch from 70c34b4 to 6b5c8d6 Compare September 18, 2023 16:43

ltrung approved these changes Sep 18, 2023

View reviewed changes

pracheth merged commit 72c7a64 into main Sep 18, 2023
10 checks passed

pracheth deleted the audio-redundancy branch September 18, 2023 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sending and receiving redundant audio data to help reduce the effects of packet loss on audio quality. #2745

Support sending and receiving redundant audio data to help reduce the effects of packet loss on audio quality. #2745

pracheth commented Aug 31, 2023

hensmi-amazon left a comment

hensmi-amazon Aug 31, 2023

pracheth Sep 5, 2023

ltrung Sep 5, 2023

hensmi-amazon Sep 7, 2023

ltrung Sep 8, 2023

hensmi-amazon Aug 31, 2023

ltrung Sep 5, 2023

pracheth Sep 6, 2023

ltrung Sep 8, 2023

pracheth Sep 11, 2023

ltrung Sep 6, 2023

pracheth Sep 6, 2023

pracheth Sep 8, 2023

ltrung Sep 6, 2023

pracheth Sep 6, 2023

pracheth Sep 8, 2023

mklingb left a comment

Support sending and receiving redundant audio data to help reduce the effects of packet loss on audio quality. #2745

Support sending and receiving redundant audio data to help reduce the effects of packet loss on audio quality. #2745

Conversation

pracheth commented Aug 31, 2023

hensmi-amazon 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

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

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

mklingb left a comment

Choose a reason for hiding this comment