14.09.2014 01:14, Jaroslav Kysela wrote:
Date 13.9.2014 20:30, Alexander E. Patrakov wrote:
The idea of the series is to fix the two issues that I found [1] for the
I applied all your patches to alsa-lib's repo, but...
hw plugin. snd_pcm_rewindable() sometimes returned negative values that are actually negative amounts of samples and not error codes. Also, it bases its calculations on stale hardware position pointer, which is not what PulseAudio wants (alternatively, we can document the need to call snd_pcm_avail() before snd_pcm_rewindable(), but I don't like it).
The hw sync is expensive and the application might do this sync multiple times when woken up. I think that it must be clear that:
- only snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay() does the real hw sync
- snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay(), snd_pcm_rewindable() and snd_pcm_forwardable() does hw sync (and change all plugins to respect this)
I don't like the situation "be somewhere between because it's good for one purpose"...
I understand the concern. I have specifically not added the call to hwsync directly to snd_pcm_rewindable implementation (although it would have resulted in a smaller patch), because that would indeed cause double-hwsync and the resulting inefficiency. I made sure that all plugins either make the hwsync thing themselves or rely on the slave to do that for them, but not both. If you find an error and/or spot a case of a double-hwsync in a plugin chain, please complain.
One known case of double-hwsync is the following pattern: an application calls snd_pcm_rewindable(), thinks about it, and then calls snd_pcm_rewind(). Which, due to PATCH 2/9, calls the rewindable callback again, resulting in the second hwsync. I don't know which way out is best: either ignore, or revert the intention of PATCH 2/9, or revert the whole PATCH 8/9 and replace it with a documentation change.
OTOH, I made a mistake of not adding David Henningsson to the CC list during the initial submission. If PulseAudio would need to synchronize hardware pointers even after conversion to snd_pcm_rewindable() for some other reason, then the need for PATCH 8/9 is not that obvious, and maybe it should be reverted and replaced with a documentation fix.