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

Aplay resampler #750

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Aplay resampler #750

wants to merge 5 commits into from

Conversation

borine
Copy link
Collaborator

@borine borine commented Feb 2, 2025

This is my attempt to address the topics previously discussed in PR #459

The io_worker_routine() function is already quite complex and can be difficult to read, so before adding more complexity, the first three commits simply factor out some of the code into new files.

The commit "aplay: improve overrun/underrun handling" uses a different approach, but addresses the same problem, as #459 and therefore if merged it closes #459

The libsamplerate resampler causes high CPU usage. Its not so bad on x86_64, but really very high on armhf. I have not tried with arm64. Having said that, I have tested on an old pi zero W (32bit armv6) with the "SINC_FASTEST" converter and got very stable audio with accurate delay reporting watching a 4 hour video stream. top showed bluealsa-aplay consuming 28% of the (single core) CPU but the pi continued to run smoothly. So anyway this is left as an option and not included in the build by default.

Copy link

codecov bot commented Feb 2, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 19 lines in your changes missing coverage. Please review.

Project coverage is 70.53%. Comparing base (f0f9427) to head (0aaf9e9).

Files with missing lines Patch % Lines
utils/aplay/alsa-pcm.c 65.85% 14 Missing ⚠️
utils/aplay/aplay.c 86.20% 4 Missing ⚠️
utils/aplay/delay-report.c 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
- Coverage   70.54%   70.53%   -0.02%     
==========================================
  Files          99       99              
  Lines       16181    16217      +36     
  Branches     2527     2531       +4     
==========================================
+ Hits        11415    11438      +23     
- Misses       4647     4660      +13     
  Partials      119      119              

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

utils/aplay/alsa-pcm.c Fixed Show fixed Hide fixed
utils/aplay/aplay.c Dismissed Show dismissed Hide dismissed
utils/aplay/aplay.c Fixed Show fixed Hide fixed
@arkq
Copy link
Owner

arkq commented Feb 5, 2025

I'm currently in the review process. If you agree, I will push the refactoring bits to the master branch and keep only the resampling-related changes here. Please give me a day or two to examine the refactoring, then I will look at the resampler. Thank you very very much for this piece of work!

@borine
Copy link
Collaborator Author

borine commented Feb 6, 2025

If you agree, I will push the refactoring bits

Yes, please just take any pieces you feel would be useful.

@arkq
Copy link
Owner

arkq commented Feb 6, 2025

I've already took first 4 commits. I've applied some polishing to them, so unfortunately, your other commits will not apply cleanly on master. But please, do not worry, I will rebase this PR on top of master once I will review the "aplay: improve overrun/underrun handling" commit.

I've updated the delay report API a little bit. Now, it gets all delay components internally (ALSA PCM delay as well). I'm not sure whether it will be OK with the rest of your work, but from this single commit point of view it is cleaner that way. The rest is not changed other than some comments updates or code style updates.

@arkq
Copy link
Owner

arkq commented Feb 6, 2025

I just reviewed the "improve overrun/underrun handling" commit and it also looks OK.
I've pushed it here: https://github.com/arkq/bluez-alsa/tree/aplay-sync

I've made some minor changes, though:

  1. I've removed delay_report_reset(&dr); from the "if" where samples are removed from the internal ffb buffer. The case is, that removing all accumulated delay values will cause delay report to be ~0 and then again accumulate to real value. I'm not sure that we want that.
  2. I've changed the debug("Read buffer overrun") to warning, because I think that drops should be visible to user to indicate that something is not right.
  3. The worker->ba_pcm.running = pcm->running; is done in supervise_io_worker_start() since this function is responsible for creating worker, so it can also maintain already created one.
  4. I think that here was an error, so I've fixed it:
    /* Pad the buffer with enough silence to restore it to the underrun
     * threshold. */
    size_t padding = (pcm->underrun_threshold - frames) * pcm->channels;
    if (verbose)
       info("Underrun imminent: inserting %zu silence frames", padding / pcm->channels);
    snd_pcm_format_set_silence(pcm->format, buffer->tail, padding);
    ffb_seek(buffer, padding);
    frames += padding;      // <---- the padding is in samples, not frames

Other than that, I've just rebased it on top of master.

I will test it tomorrow and if everything is OK I will push that to master (if you do not have any objections/remarks).

@borine
Copy link
Collaborator Author

borine commented Feb 7, 2025

removing all accumulated delay values will cause delay report to be ~0

yes, not good from the client view. The same is also true of the reset after an ALSA underrun. When I put those lines in I was thinking only of the resampler, needing some way to indicate that the delay report needed time to settle on a new average before using to calculate a rate adjustment. I agree that clients will benefit by removing the reset() calls, and the resampler management needs a bit more work.

@arkq
Copy link
Owner

arkq commented Feb 7, 2025

I've rebased the resampler on top of master. However, I'm not sure whether I have not broken anything along the way... Also, I have not reviewed changes yet, it's only a bling rebase attempt. Over the weekend I will try to do some preliminary review of all these changes.

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