Cleanup, optimization, and bug fixes #138
Conversation
- when requesting a packet, do not request an already received packet - move the last-chance resend to buffer_get_frame - check more often for missing frames own checks show that some frames need to be requested up to 4 times on bad connections
Make the dependency and compile command match in the resources used.
This makes things a bit cleaner and easier to grasp.
This allows the compiler to do a much better job on this file, as it currently can't inline most of the functions because they are technically visible outside the file. Mark most functions and variables static to let the compiler work.
If you turned set `debug = 1` in hairtunes, you'd quickly get a mess of debug messages that showed bf_est_drift in bf_est_update() going quicky out of range toward a float NaN value (usually negative). Clearly the presence of this `out` variable in biquad_filt was meant to be used, not marked as unused.
Using the `volatile` qualifier in multithreading code is never the right answer. Mutexes should be used as was attempted with the audio buffer code. Here, we implement a new mutex for the volume and fix_volume globals, and grab a lock on it when necessary, which is for both reads and writes.
We need to use this any time we write to or access any of the audio-buffer related variables. This also removes the need to use 'volatile', which cripples the compiler optimizations.
Most functions don't need to be public outside of this file, so move all the forward declarations out of shairport.h.
Cleanup, optimization, and bug fixes
|
Thanks for all the cleanups! I don't see how using a mutex (for eg. the volume control) has any bearing on marking things volatile. Certainly I'm a little over-eager with volatile (courtesy of a lot of pain with embedded compilers), but I wasn't aware of any mutex semantics that cause the compiler to reconsider its variable values! Also, there's no point to using a mutex with the volume control at all - it doesn't matter whether it changes mid-block or not. |
|
Calling pthread_mutex_lock() and unlock() guarantee memory synchronization: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11 Right now, you are doing something absolutely silly- reading from the volatile variable 'volume' in a tight loop, which requires a memory access every single time, as the compiler is held hostage and can't just store that value in a register. volatile is like using a bomb when you need a flyswatter; it has some very bad consequences, and I'm still not sure it actually gives you the guarantees you are looking for. So in short, the mutex is much much cheaper than the volatile in all respects. |
|
Interesting. Thank you. |
The main big points here:
This also includes the one commit from Stef's outstanding pull request.
https://github.com/albertz/shairport/pull/131