2013/7/11 Peter Meerwald <pmeerw at pmeerw.net>: > Hello, > >> > This commit seems to break speech-dispatcher: >> > http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=30ce3a14e5ae1cd316a18bec95b831c07ac57a1a > > this is not good > >> buffer handling. Converting o_ss.channels to work_channels isn't enough >> to fix the issue with speech-dispatcher, however, because there's also >> another thing issue: when resampling happens before remapping, the >> leftover buffer handling is completely broken, because it assumes that >> the leftover data from the resampling phase is supposed to be stored in >> the remap buffer, while it actually should be stored in the >> to_work_format buffer. At least the ffmpeg resampler needs the leftover >> handling with the speech-dispatcher stream. I don't have a simple fix >> for this, so I'll just revert the patch for now. > > the ffmpeg and libsamplerate code use the leftover logic; I assume > speech-dispatcher selects ffmpeg or libsamplerate instead of the default > speex? or is the bug due to the issue in the peaks resampler? > > Tanu, do you know the constellation that triggers the bug? is #output > channel > #input channels? > > > a resampler implementation has several options wrt input and output buffer > use: > (i) it allows short reads, i.e. it is free not to use all samples given > in the input buffer > (ii) it allows short writes, i.e. it is free to not write all samples > requested to the output buffer > (iii) both, (i) and (ii) > > save_leftover() is necessary if the resampler does (i) > > doing (ii) is much less painful for PA, there is just more or less (maybe > zero) output sometimes > >> It's by no means impossible to add support for storing the leftover data >> in the to_work_format buffer, if someone wants to work on it, but the >> leftover code is hairy already as it is, and this would make it doubly >> so. I will accept patches that do that, although it would be better if >> the leftover buffer handling could be simplified. I'm not sure if it's >> possible to simplify the code without affecting the performance, though. >> I tried pretty hard when I originally wrote the leftover logic. That >> said, perhaps a minor performance hit would be acceptable just for code >> readability. > > maybe we can get rid of the leftover logic altogether: > * ffmpeg is going to be replaced by libswresample / livavresample; > swr_convert() [1] takes all input and buffers it, hence is not type-(i) > * libsamplerate offers three APIs: simple, full, and callback [2]; it > seems the callback interface could be used which is not type-(i) > > nevertheless, I think any type-(i) resampler could be relaxed (made more > convenient for PA) by implementing a wrapper to add buffering and not > pushing back samples to the remap buffer (maybe this is what you meant by > simplifying the leftover buffer handling?) > > I'd be interested in working in that direction Please bear in mind that there is yet another manifestation of the problem that you stated above. Namely, pulseaudio sometimes rewinds its audio buffers in order to amend something it wrote there before. In order for such amending to work correctly, pulseaudio must ensure that there is no reuse of the leftover samples across the rewind. This also applies to your wrapper, otherwise the following happens: pulseaudio pushes some samples to the resampler, resampler (or wrapper) remembers the last few of the fed-in samples because they would be needed for the next portion, but then pulseaudio feeds in a completely unrelated block of audio instead of that portion because of the rewind. I am not even sure that all included resamplers handle rewinds correctly (but I have not checked). Feel free to audit them. What I know for sure is that the virtual surround sink does this incorrectly (zeroes its internal left-over buffer on rewind instead of asking for prior input samples or indicating that some output samples have to be discarded as known-incorrect). -- Alexander E. Patrakov
