Improve Alsa backend buffer slightly - #811
Conversation
|
Thinking about the case where there isn't enough data to feed into the if self.period_buffer.len() == self.period_buffer.capacity() || processed_data == data.len() { But I don't think we should. First, when there will be more packets fed afterwards (which |
I'm not really sure it's even a concern. That would mean that the decoder could not keep up with the PCM. That would indicate either the device isn't powerfull enough to run |
Currently the buffer is recreated every time start is called. This also fixes a previous commit's mistake of making it so the buffer is not drained on stop.
JasonLG1979
changed the title
* As decided in #811 (comment) don't absolutely depend on `Vec` capacity or growth strategies. Use the period size as the authority not capacity. * Continuing on the theme of not trusting `Vec` growth strategies, over allocate initially so that our Vec will never actaully reach full capacity thus (hopefully) avoiding future reallocations. * Move all buffer code to impl AlsaSink. * Add a new (IMHO more legible) recursive `process_bytes` function for chewing though the bytes sent to us from mod. * Other small cleanups.
And add edit some comments for clarity and typos
|
@roderickvd I'd also like some more reviews of this. It started out as just simply reusing the buffer but turned into a refactor because we came to the conclusion that basically |
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
Now that you've proven Vec::with_capacity(x) does what is says on the tin, unlike resize(x), there are some simplifications that can be made.
I like you addressed that tiny drain bug en passant.
This basically narrows this PR down to: * Use a loop expression instead of a while loop to chew though the bytes because it's faster. * Always give the PCM a period's worth of audio even when draining the buffer. This makes for more consistent stop to start latency (pause to play, manually changing tracks) and helps to prevent jarring, "chopped off" sounding transitions when changing tracks manually. Overall resulting in a slightly smoother user experience.
JasonLG1979
changed the title
|
I think that covers everything. Addressing everything narrowed the scope of the PR A LOT. |
|
If anyone is curious as to how I came to the conclusion that a |
Indeed! I'll do some final sanity checking myself and then we can get this in.
Having fun with Rust yet? 😄 |
I am. I just know that if I'm going to claim something I better be able to back it up. |
|
I googled it and couldn't find any definitive answers so I figured that mocking up a test that simulates our use case was the best way to find out the truth. Again I tested on x86_64, ARMv6 and ARMv8. The result was the same on all of them, loop is about 3 to 5% faster. And as a bonus it's the most idiomatic of the 3 IMHO. |
|
Doing some final checking on my own rig now, will merge on success. |
|
Unfortunately it panics when playing to |
No that's what happens when you just copy and paste diffs instead of actually checking the code out and you forget |
|
It to be fair, I've done the same on accident and it took me a little bit to realize my copypasta mistake,lol!!! Edit: I thoroughly tested this against |
|
|
|
I'm listening to this branch right now on my Pi Zero though |
|
Here is my current And since I specify |
|
And if you wanted to skip That outputs the same for Notice that |
|
So I just checked: it's a clean checkout @ 85fb588 with that It still panics for me when launched simply as
|
|
With I have no idea what's going on (though I haven't dug deep into your code either) but having opened I'll fire up some older Linux box to see if I can reproduce it there. But this is definitely fishy. |
|
Same on my old 2010 MacBook Pro that now runs elementary OS (Ubuntu 18.04 LTS). It works fine on Only thing in |
|
I can not reproduce any of those issues on my Pi Zero. I will dig a little deeper after work. The only panic I got was once from mdns? You would not normally use
Stupid question, but did you turn it up? Because I thought "Wow, what's going on? I don't have any sound either" then I noticed the volume was at 30 so I turned it up and it was working just fine. |
|
To be clear I tested with |
|
And as to be expected |
I did. Also 30% is audible for me (I've got an amp with a lot of headroom), as shows when I play on
Note that my I'll have to do some debugging myself to work this one out. |
|
I'm back. i'm starting my deep dive. I think at the very least no matter what happens I going to add a few debug!("AlsaSink Frames per Buffer: {:?}", frames_per_buffer);
debug!("AlsaSink Frames per Buffer Period: {:?}", frames_per_period);
debug!("AlsaSink Buffer size in bytes: {:?}", bytes_per_period);They may be helpful now and in the future. |
|
@roderickvd I apologize for being kinda an ass. You were right. I think that I figured it out. Give that a try. |
|
Okay wow, I see #811 (comment) opened up a can of worms! |
Well any question that leads to learning something is a good question. |
Vec.shrink_to_fit and Vec.reserve_exact have proven to be reliable well past our needs. (https://gist.github.com/JasonLG1979/69ed65d91c64f39de4435b3720258d02) There's no reason to not reuse the buffer and avoid unnecessary allocations.
|
No worries, good you got this sorted out. It's working fine now for me. Ready to merge everyone? |
If by "everyone" you mean you and I, sure. Because I don't think anyone else cares,lol!!! |
|
At least I try to keep up appearances of democracy 😆 and give everyone the chance to look at something that will potentially be used by quite a large share of users. I was thinking if we needed the chatter of the debug statements on every sink start/stop, and if there even was a case where this would be different between tracks. While I guess it could be done (calling an Thanks! |
That's fine. I just wanted it to be easily available to users to aid in debugging in issues. Trace works. |
In the future if Spotify were to ever offer tracks in different formats, channel counts, and sample rates there's a possibility that basically everything could change in between tracks. If that happens we will HAVE to At some point if track specs were to change in between tracks we will have to implement some form of resampling so we can still output a consistent format and sampling rate no matter what is thrown at us to avoid situations where All of this is just pure musing and speculation though. As it is now realistically nothing changes for the life of |
|
https://github.com/lrbalt/libsoxr-rs might be worth a look? I'm not sure if it's cross-platform but SoX is pretty well regarded on Linux for resampling. |
|
We shortly discussed options for resampling on the now-closed PR with the I'd really like to steer away from such sound processing unless absolutely necessary. Even then, we would be approaching what Rodio is doing and it seems pointless to reinvent the wheel. From the latest protobufs it seems clear that Spotify HiFi will be using FLAC. Earlier press statements said "CD-quality". Assuming that is 44.1 kHz 16-bit, we'll be fine without a resampler. I've done some preliminary works on getting |
I agree. Avoiding as much processing as possible is the way to go. I was just thinking out loud. It's really one of those "we'll cross that bridge when we get to it" type situations.
|
* Reuse the buffer for the life of the Alsa sink * Don't depend on capacity being exact when sizing the buffer * Always give the PCM a period's worth of audio even when draining the buffer * Refactoring and code cleanup
3 participants
Reuse the period buffer.
Vec.shrink_to_fitandVec.reserve_exacthave proven to be reliable well past our needs. (https://gist.github.com/JasonLG1979/69ed65d91c64f39de4435b3720258d02)There's no reason to not reuse the buffer and avoid unnecessary allocations.
Use a loop expression instead of a while loop to chew though the bytes because it's faster.
(https://gist.github.com/JasonLG1979/3efb5646e4bb42309bdda6d83a3da54e)
Always give the PCM a period's worth of audio even when draining the buffer.
This makes for more consistent stop to start latency (pause to play, manually changing tracks) and helps to prevent jarring, "chopped off" sounding transition when changing tracks manually. Overall resulting in a slightly smoother user experience.