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

Add local volume control #468

Merged
merged 12 commits into from
Jul 28, 2022
Merged

Conversation

SimonBrandner
Copy link
Contributor

@SimonBrandner SimonBrandner commented Jul 14, 2022

Requires matrix-org/matrix-js-sdk#2525


During calls it can often happen that the audio coming from one person is louder than the audio coming from another. This can of course be fixed on the sending side but it's often easier to just change the volume levels locally. This is especially helpful during calls with screen-sharing where audio is also shared to control if the user wants to hear the screen-sharing audio (or how loud they want it to be)


This needs product to take a look


Peek 2022-07-15 11-16

Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
@SimonBrandner SimonBrandner marked this pull request as ready for review July 15, 2022 11:17
@SimonBrandner SimonBrandner requested a review from a team as a code owner July 15, 2022 11:17
@robintown
Copy link
Member

This is of course up to Product, but I'd suggest the ability to go above 100% as well to deal with quiet speakers (just mentioning it here for completeness)

@robintown robintown requested a review from a team July 15, 2022 11:57
@jakewb-b
Copy link

This is great - I'm definitely in favour.

This is of course up to Product, but I'd suggest the ability to go above 100% as well to deal with quiet speakers (just mentioning it here for completeness)

I agree with being able to turn it up. Referring to it as 'above 100%' feels a bit awkward to me and might be confusing, although I assume it means artificially boosting the volume. Maybe we're better off just starting the slider in the middle and then either not having numbers as you move it left and right, or having it something more arbitrary like sliding left goes down to -10, and sliding right goes up to +10. Ideally that's something @gaelledel could take a look at.

Can I also just confirm, is the volume setting for a given user maintained if:

  • That user drops from this call and rejoins it almost immediately (e.g. within 5 minutes)?
  • Rejoins the same call on a later date (i.e. it's a weekly call in the same room, with the same people)
  • Joins another call with me?

In the first case, it might be good to have the setting maintained, but in other cases I'd er towards re-setting it to the middle each time.

@SimonBrandner
Copy link
Contributor Author

Can I also just confirm, is the volume setting for a given user maintained if:

  • That user drops from this call and rejoins it almost immediately (e.g. within 5 minutes)?
  • Rejoins the same call on a later date (i.e. it's a weekly call in the same room, with the same people)
  • Joins another call with me?

In the first case, it might be good to have the setting maintained, but in other cases I'd er towards re-setting it to the middle each time.

Currently, the volume is only maintained during the call. It should be possible to persist the volume per-user per-call though I am not sure the time reset is the way to go - it would be a little difficult to implement and I think other VoIP software (such as Discord) persists the value independent of time. Though that is of course just my POV on this

@jakewb-b
Copy link

Yeah, I'm a little undecided on whether we'd want it to persist per-user per-call. On the one hand, if a given user is always too quiet, and I always have to turn them up, that would be annoying.
On the other hand, if I've once turned a user all the way down because they were too loud, it would be easy to forget that and think their audio was broken when they joined next week's call. Or, conversely, have them suddenly blasting audio at high volume because last call I turned them all the way up.

Let's leave that for now and stick with re-setting each time. I think we could re-visit this when it gets more design attention, as maybe the solution is more of a visual indicator on a user if they've had their audio adjusted away from the default in either direction.

Copy link

@jakewb-b jakewb-b left a comment

Choose a reason for hiding this comment

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

I'm happy with this from the product side. Once Gaelle has capacity, it would be good for her to review this and make any recommendations for further changes, but for a fairly minor piece of setting UI I think we can do that in the future once it's live.

src/button/Button.tsx Outdated Show resolved Hide resolved
src/video-grid/VideoTileSettingsModal.module.css Outdated Show resolved Hide resolved
src/video-grid/VideoTileSettingsModal.module.css Outdated Show resolved Hide resolved
src/video-grid/useMediaStream.ts Show resolved Hide resolved
src/video-grid/useMediaStream.ts Show resolved Hide resolved
src/video-grid/VideoTileSettingsModal.tsx Outdated Show resolved Hide resolved
src/video-grid/VideoTileSettingsModal.tsx Show resolved Hide resolved
src/video-grid/VideoTileSettingsModal.tsx Outdated Show resolved Hide resolved
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

:shipit:

@SimonBrandner SimonBrandner merged commit 942800a into main Jul 28, 2022
@SimonBrandner SimonBrandner deleted the SimonBrandner/feat/local-volume branch July 28, 2022 16:09
@gaelledel
Copy link
Collaborator

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.

4 participants