arkq / bluez-alsa

Bluetooth Audio ALSA Backend
MIT License
864 stars 189 forks source link

Delay adjustment #653

Closed borine closed 1 year ago

borine commented 1 year ago

See discussion #650 for rationale. Extends the D-Bus API org.bluealsa.PCM1 to allow setting a persistent adjustment that is applied to the Delay property of the PCM. Each codec supported by the PCM has its own delay adjustment.

The choice of control name for the CTL plugin is not yet agreed, and in this initial draft the name "Sync" is used.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 41.10% and project coverage change: -0.81% :warning:

Comparison is base (4c3c49d) 71.08% compared to head (75169e5) 70.27%.

:exclamation: Current head 75169e5 differs from pull request most recent head 449fa24. Consider uploading reports for the commit 449fa24 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #653 +/- ## ========================================== - Coverage 71.08% 70.27% -0.81% ========================================== Files 47 48 +1 Lines 9010 9239 +229 ========================================== + Hits 6405 6493 +88 - Misses 2605 2746 +141 ``` | [Files Changed](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy) | Coverage Δ | | |---|---|---| | [utils/cli/cmd-delay-adjustment.c](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy#diff-dXRpbHMvY2xpL2NtZC1kZWxheS1hZGp1c3RtZW50LmM=) | `0.00% <0.00%> (ø)` | | | [src/shared/dbus-client.c](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy#diff-c3JjL3NoYXJlZC9kYnVzLWNsaWVudC5j) | `66.32% <12.50%> (-1.97%)` | :arrow_down: | | [src/bluealsa-dbus.c](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy#diff-c3JjL2JsdWVhbHNhLWRidXMuYw==) | `65.06% <13.63%> (-3.57%)` | :arrow_down: | | [src/asound/bluealsa-ctl.c](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy#diff-c3JjL2Fzb3VuZC9ibHVlYWxzYS1jdGwuYw==) | `75.31% <45.61%> (-1.84%)` | :arrow_down: | | [src/storage.c](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy#diff-c3JjL3N0b3JhZ2UuYw==) | `83.95% <83.72%> (-0.09%)` | :arrow_down: | | [src/ba-transport.c](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy#diff-c3JjL2JhLXRyYW5zcG9ydC5j) | `79.79% <100.00%> (+0.48%)` | :arrow_up: | | [utils/cli/cli.c](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy#diff-dXRpbHMvY2xpL2NsaS5j) | `77.55% <100.00%> (+0.09%)` | :arrow_up: | ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/Arkq/bluez-alsa/pull/653/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Arkadiusz+Bokowy)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

borine commented 1 year ago

I've had second thoughts about the non-blocking D-Bus calls, because amixer does not wait for the update to complete and therefore does not report the updated value. So I've now reverted that commit and instead invoke the D-Bus event handling code explicitly after codec and sync updates. This approach seems to keep both amixer and alsamixer happy.

I plan to rebase against master this weekend to pull in the revised EXT argument code, and at the same same squash some of these commits to make each logical change a single commit. I will then mark this PR as ready for review.

BTW I've been searching through dictionaries and thesauruses to find better names for the delay adjustment control, but given there is only room for 4 characters I still haven't found anything better than "Sync".

arkq commented 1 year ago

BTW I've been searching through dictionaries and thesauruses to find better names for the delay adjustment control, but given there is only room for 4 characters I still haven't found anything better than "Sync".

OK, I think that we can go with "Sync" then. For audio only usage it's a little bit cryptic name, but since this control element will not be visible by default it's OK. And for A/V usage, I guess that someone who will need to adjust sync issue the "Sync" word in the manual/wiki will help to find the answer :)

arkq commented 1 year ago

I've just reviewed commits for CTL plugin and for bluealsa-cli, and they look good. I've fixed some typos and reordered sync and battery (battery is the least related with audio, so I'd like to place it as the last item) and force pushed the changes (to avoid conflicts in the nearest future).

Now, I need to find some free time to review the core part of this PR... However, already I've spotted one thing that I'd like to change. The ba_transport_pcm_delay_adjustment_set should get the codec_id not a string representation. It will be consistent with the rest of the internal API.