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(bnr): add getEffects and modify disposeEffects #43

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

lismith2-cisco
Copy link
Contributor

Need a better interface for applying effects for implementing bnr in webex web client

@bbaldino
Copy link
Collaborator

Out of curiosity, what's the use case to get a map of all the effects?

@lismith2-cisco
Copy link
Contributor Author

Out of curiosity, what's the use case to get a map of all the effects?

When selecting 'no optimization' I want to disable any current effect (but not dispose it) so it can be re-enabled quickly. There were a couple of options for how to do it, but this seemed best.

@bbaldino
Copy link
Collaborator

When selecting 'no optimization' I want to disable any current effect (but not dispose it) so it can be re-enabled quickly. There were a couple of options for how to do it, but this seemed best.

@shaofan-qi I have a vague memory of you finding some issues around disabling but not "removing" effects, am I remembering that right? Should effects be removed completely when they're not in use? cc @andiwils who might have some thoughts as well.

@bbaldino
Copy link
Collaborator

bbaldino commented Apr 13, 2023

Some comments from @andiwils offline:

If they will never be used again, they should be disposed of. With VBG, disabling still ate up a bunch of CPU until recently when we decided to just swap the original track with the effect track. We are not doing that with BNR yet but plan to.
In other words, disabling should have similar performance to not using the effect at all. That much is true with VBG today. Quality shouldn't be affected.

So sounds like even if this might currently have some issues, it's how things should work, but heads up on the potential caveats andi mentioned @lismith2-cisco.

@bbaldino bbaldino merged commit f5117f2 into webex:main Apr 13, 2023
github-actions bot pushed a commit that referenced this pull request Apr 13, 2023
# [1.6.0](v1.5.1...v1.6.0) (2023-04-13)

### Features

* add getEffects and fix disposeEffects to be async ([#43](#43)) ([f5117f2](f5117f2))
@shaofan-qi
Copy link
Contributor

The issue we met was about BNR in Safari. Even the effect was disabled, it resulted in stuttering audio. We may need to look into what's going wrong with the BNR effect. If I remember right, the issue disppears in some newer Safari versions (16.0+) on Intel Macs, but still exists on Apple Silicon Macs, but we haven't tested that for a while.

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.

3 participants