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

bluealsa-aplay: option to select volume mode #664

Merged
merged 1 commit into from
Sep 2, 2023

Conversation

borine
Copy link
Collaborator

@borine borine commented Sep 1, 2023

A proposal to improve bluealsa-aplay volume control. Inspired by the discussion in PR #662, this proposal gives the bluealsa-aplay utility the ability to select soft volume or pass-through volume control independently of the bluealsa service defaults.

Default behavior remains unchanged, so backwards compatibility for existing installations is maintained.

See the changes to doc/bluealsa-aplay.1.rst for details of the new command-line option and the various new behaviors made available.

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch coverage is 40.00% of modified lines.

❗ Current head cc8c0ca differs from pull request most recent head 3a820d8. Consider uploading reports for the commit 3a820d8 to get more accurate results

Files Changed Coverage
utils/aplay/aplay.c 40.00%

📢 Thoughts on this report? Let us know!.

arkq
arkq previously approved these changes Sep 1, 2023
Copy link
Owner

@arkq arkq left a comment

Choose a reason for hiding this comment

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

It's hard for me to judge the user experience with such --volume= option (volume control is not my every day use case here), but from the code point of view it looks great!

doc/bluealsa-aplay.1.rst Outdated Show resolved Hide resolved
utils/aplay/aplay.c Outdated Show resolved Hide resolved
utils/aplay/aplay.c Show resolved Hide resolved
utils/aplay/aplay.c Outdated Show resolved Hide resolved
utils/aplay/aplay.c Outdated Show resolved Hide resolved
@borine
Copy link
Collaborator Author

borine commented Sep 2, 2023

volume control is not my every day use case

To be honest, nor is it mine. I do not use bluealsa-aplay with SCO, and I always use software volume control with A2DP. Given how few issues/feature requests/PRs there are relating to bluealsa-aplay I think my use case is possibly the most common, and those users/distributions who do use mixer volume control seem to be generally satisfied with the existing implementation. It's at this point in discussions that I always suggest that we solicit opinions from the user community; but we know from experience that rarely generates much interest.

The motivation for this PR comes from PR #662, and is three-fold:

  1. The creator of PR Fix adjust volume invalid in using soft volume for hfp #662 @xiaoyao888888 wishes to use bluealsa-aplay with SCO, and have software volume control by default. This PR offers an alternative solution to that requirement, and I would be very interested to hear any comments from @xiaoyao888888 on his opinion of the relative merits of both solutions for his own specific use case.

  2. In PR Fix adjust volume invalid in using soft volume for hfp #662, you hinted that you may at some point in the future switch A2DP volume control to native by default, which would be inconvenient for my own use case. So this PR is a blatantly selfish attempt to pre-empt such a change by having some mitigation already in place.

  3. In order to make the solution as general as possible, I tried to imagine other likely use cases, and came up with the "auto" and "none" options as potentially useful, but without any concrete evidence that any real use cases exist.

I've pushed a new commit to address the review comments so far, and am happy to make further changes is requested.

@arkq
Copy link
Owner

arkq commented Sep 2, 2023

In order to make the solution as general as possible, I tried to imagine other likely use cases, and came up with the "auto" and "none" options as potentially useful

Yes, I think that this feature provides very generic solution without much complexity. I'll play with it a bit and I think it's ready to go to master. Many thanks for that!

arkq
arkq previously approved these changes Sep 2, 2023
@arkq arkq merged commit 3a820d8 into arkq:master Sep 2, 2023
19 checks passed
@borine borine deleted the aplay-volume-mode branch September 3, 2023 07:05
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.

2 participants