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

feat(callingsdk): initial changes to add bnr #3120

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

Kesari3008
Copy link
Contributor

@Kesari3008 Kesari3008 commented Oct 4, 2023

COMPLETES #SPARK-433414

This pull request addresses

Adding changes to use the BNR feature in CallingSDK for WebRTC Calling.

by making the following changes

Added static function to create and return NoiseReductionEffect object in calling.js
Added the effect to the localMicrophoneStream being used for calls in app.js and enabled it.
Listening for 'output_track_change' event to update the tracks once BNR is enabled/disabled.

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios where tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

I certified that

  • I have read and followed contributing guidelines

  • I discussed changes with code owners prior to submitting this pull request

  • I have not skipped any automated checks

  • All existing and new tests passed

  • I have updated the documentation accordingly


Make sure to have followed the contributing guidelines before submitting.

@Kesari3008 Kesari3008 added the validated If the pull request is validated for automation. label Oct 4, 2023
@aws-amplify-us-east-2
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-3120.d3m3l2kee0btzx.amplifyapp.com

@Kesari3008 Kesari3008 marked this pull request as ready for review October 4, 2023 11:09
@Kesari3008 Kesari3008 requested a review from a team as a code owner October 4, 2023 11:09
Copy link
Contributor

@Shreyas281299 Shreyas281299 left a comment

Choose a reason for hiding this comment

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

The changes look good. Just one minor comment

@webex webex deleted a comment from shreyas-28 Oct 5, 2023
Logger,
NoiseReductionEffect,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to export NoiseReductionEffect what is its use exporting?

Copy link
Contributor Author

@Kesari3008 Kesari3008 Oct 5, 2023

Choose a reason for hiding this comment

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

If we don't export, it will not be available to be used in samples page or by developer also

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought developers will use this method to create the NoiseReductionEffect: static createNoiseReductionEffect(authToken) . What is this about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's what developers are using. But did you see what's happening inside that static function? We are using NoiseReductionEffect inside that function. and to get access of the same, it needs to be re-exported from index.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

I did see. Just noticed the it was a js file. Lack of any return type in the signature and description confused me. Nevermind.

Copy link
Contributor

@BhargavSatya BhargavSatya left a comment

Choose a reason for hiding this comment

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

Approved With a minor comment.

@Kesari3008 Kesari3008 merged commit f28edb2 into webex:next Oct 5, 2023
8 of 12 checks passed
Kesari3008 added a commit to Kesari3008/webex-js-sdk that referenced this pull request Oct 9, 2023
Co-authored-by: Priya Kesari <pkesari@cisco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validated If the pull request is validated for automation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants