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

Enable dbus delay signals for internal processing delay. #734

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

borine
Copy link
Collaborator

@borine borine commented Oct 29, 2024

I've raised this as a draft PR for discussion because:

  1. I'm not sure if there is a reason why D-Bus signalling was omitted originally.
  2. I'm not sure of the GIO dbus thread handling. This solution emits dbus signals from the transport I/O thread, but without any locking. Is that safe?
  3. My choice of a 10ms difference threshold to limit the rate that signals can be emitted is entirely arbitrary with no specific evidence to justify it.

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 92.94118% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.42%. Comparing base (a6251b4) to head (9819781).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/ba-transport-pcm.c 81.81% 4 Missing ⚠️
test/test-a2dp.c 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
- Coverage   70.46%   70.42%   -0.05%     
==========================================
  Files          96       96              
  Lines       16050    16103      +53     
  Branches     2517     2522       +5     
==========================================
+ Hits        11310    11340      +30     
- Misses       4621     4644      +23     
  Partials      119      119              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arkq
Copy link
Owner

arkq commented Oct 29, 2024

I'm not sure if there is a reason why D-Bus signalling was omitted originally.

It was omitted due to lack of time to implement that :D Many thanks for that!

I'm not sure of the GIO dbus thread handling. This solution emits dbus signals from the transport I/O thread, but without any locking. Is that safe?

Yes, it should be safe. Internally signal is just a D-Bus message, and messages are sent by g_dbus_connection_send_message which is thread safe.

My choice of a 10ms difference threshold to limit the rate that signals can be emitted is entirely arbitrary with no specific evidence to justify it.

I'm going to use such delay for the client-delay signaling as well and it's also arbitrary decision (based on commersial headset delay report updates). However, during testing, I've also added time-rate limiting logic (https://github.com/arkq/bluez-alsa/pull/516/files#diff-8bfb2124e7db1e5681c384f69b807d2a22821e0db287cab13d9ea0eb000cff81R827) because during playback start I could unnecessarily flood other device with lots of delay reports (the 1s rate limit is also based on some commercial headset). It's implemented on the client side, but maybe it should be moved to the bluealsad... I'll have to check how this delay report signaling will work with your PR. I hope that the client-delay will be merged soon (after support on BlueZ side will fully land in upstream).

src/a2dp-aac.c Outdated Show resolved Hide resolved
@borine borine marked this pull request as ready for review December 22, 2024 18:41
@arkq
Copy link
Owner

arkq commented Dec 22, 2024

I can see that you've used ba_transport_pcm_delay_sync to notify everyone about delay change, but the ba_transport_pcm_delay_sync function does not have any hysteresis/throttling for sending D-Bus signals (it has that for updating BlueZ Delay property). I think that at least some kind of hysteresis might be required there otherwise we will DDoS D-Bus daemon :D

arkq
arkq previously approved these changes Jan 3, 2025
@arkq
Copy link
Owner

arkq commented Jan 3, 2025

During testing I've discovered one issue (thanks to this PR). The processing delay calculated in the encoder loop takes into account the BT write time, which in case of BT connection instability might increase much (up to BT connection timeout). So, before pushing this into the master I will have to think about changing the place where the delay is calculated (maybe it should be calculated before BT write, to account codec processing only, since it's named processing_delay_dms :D). The case is, that right now, when the BT socket is almost full, the "processing_delay_dms" value jitters beyond 10ms which still floods D-Bus. However, the logic in this PR is OK for me as it is.

@arkq
Copy link
Owner

arkq commented Jan 16, 2025

After some thinking about the encoder processing delay, I've ended up with a redesign of what it actually is. The summary of that is here:

bluez-alsa/src/a2dp-sbc.c

Lines 263 to 268 in b4cc4f1

/* Get the codec processing delay, which is a time spent in the
* processing loop between reading PCM data and writing the first
* encoded SBC frame. Subsequently encoded frames do not contribute
* to the delay, because (assuming no underruns) since the first
* frame is written, the BT sink can start decoding and playing
* audio in a continuous fashion. */

I've rebased your PR on top of that.

Make sure that clients are informed of significant changes to the
decoder internal processing delay, even when used with remote
devices that do not report their own delay.
@arkq
Copy link
Owner

arkq commented Jan 17, 2025

Due to redesign of processing delay calculation, I've simplified your changes a little bit. I think that now it's ready to go to master.

@arkq arkq merged commit 9819781 into arkq:master Jan 17, 2025
21 checks passed
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