-
Notifications
You must be signed in to change notification settings - Fork 193
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
It was omitted due to lack of time to implement that :D Many thanks for that!
Yes, it should be safe. Internally signal is just a D-Bus message, and messages are sent by
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). |
79c4b9c
to
25a412d
Compare
I can see that you've used |
924eb34
to
8710c39
Compare
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 |
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: Lines 263 to 268 in b4cc4f1
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.
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. |
I've raised this as a draft PR for discussion because: