illuusio / portaudio

PortAudio is a cross-platform, open-source C language library for real-time audio input and output. This repo adds out-of-tree Pulseaudio support.
Other
1 stars 1 forks source link

Timestamping broken in your current Pulseaudio branch. #10

Closed kleinerm closed 1 year ago

kleinerm commented 2 years ago

Not sure why, but at least on Ubuntu 20.04.4 LTS with your new thread-based implementation, timestamping is broken, and my application only outputs silence, because pa_stream_get_time( s, &l_lStreamTime )

in PaPulseAudio_updateTimeInfo() now only reports a l_lStreamTime of zero.

So something is not quite right anymore.

illuusio commented 2 years ago

As I believed that Autotime stamping is working. It seems not. I'll revert things... :disappointed:

illuusio commented 2 years ago

Ok I checked with current Git and latest Pulseaudio (which not the case with Ubuntu 20.04.4) and it worked as expecte. If you could remove last 2 commits with git reset --hard and test if they were causing the trouble or rebase them a way. I'll test on Ubuntu 18.04 later on which has older Pulseaudio

kleinerm commented 2 years ago

The broken timestamping may just be a side effect, because at least my app also does not play any sound. If the stream would not actually start, reporting zero time would probably be expected. I do not get any errors though, just silence and zero timestamps.

If i revert the new threaded implementation ie. revert commits "PulseAudio: Add return and trust there is automatic timestamps" and "PulseAudio: Start using thread", then sound and timestamps work again. Only reverting "PulseAudio: Add return and trust there is automatic timestamps" does not help, only reverting "PulseAudio: Start using thread" does make it work again.

So it is "PulseAudio: Start using thread" or some preparatory commit leading up to that?

pactl --version reports these version for PulseAudio: pactl 13.99.1 Compiled with libpulse 13.99.0 Linked with libpulse 13.99.0 pactl info gives: Server String: /run/user/1000/pulse/native Library Protocol Version: 33 Server Protocol Version: 33 Is Local: yes Client Index: 97 Tile Size: 65472 User Name: kleinerm Host Name: touchy Server Name: pulseaudio Server Version: 13.99.1 Default Sample Specification: s16le 2ch 44100Hz Default Channel Map: front-left,front-right Default Sink: alsa_output.pci-0000_00_1f.3.analog-stereo Default Source: alsa_input.pci-0000_00_1f.3.analog-stereo Cookie: 6348:dc7a

kleinerm commented 2 years ago

Ok, did some more debugging:

I get silence because the paCallback always requests 0 new frames in callback mode. This is because l_ptrStream->bufferProcessor.framesPerHostBuffer is zero inside the new threading implementation. It is that because PaUtil_InitializeBufferProcessor() initializes with a framesPerHostBuffer = framesPerBuffer and user provided framesPerBuffer is zero. However if i provide a non-zero value then it also doesn't work.

What i see in the new threaded code is that: PaUtil_SetOutputFrameCount( &l_ptrStream->bufferProcessor, l_ptrStream->bufferProcessor.framesPerHostBuffer );

where l_ptrStream->bufferProcessor.framesPerHostBuffer is always zero, but in the old code, that was

PaUtil_SetOutputFrameCount( &l_ptrStream->bufferProcessor, length / l_ptrStream->outputFrameSize ); and length was an input parameter for the PaPulseAudio_StreamPlaybackCb which no longer exists.

So the old implementation got the info from Pulseaudio, but the new one doesn't afaict.

illuusio commented 2 years ago

Ok situation is that there is no request for framesPerHostBuffer and it's zero or below? Then is has to be made up some number for framesPerHostBuffer and it should work.

illuusio commented 2 years ago

Could you test with turning of everything else than Pulseaudio. With CMake

mkdir build
cd build
cmake -DPA_USE_ALSA=OFF -DPA_USE_OSS=OFF -DPA_USE_JACK=OFF -DPA_BUILD_EXAMPLES=ON ..
examples/paex_sine

And test if examples/paex_sine gives output. Then I just needs small adjustment.

kleinerm commented 2 years ago

Nope, just gives silence, exactly like with my own portaudio client application, despite the framesPerBuffer variable in paTestCallback reporting the expected 64 farmes per callback invocation. Also, pactl list sinks does show the sink going RUNNING while paex_sine is running, and the Ubuntu sound settings GUi shows paex_sine as additional client application playing sound.

kleinerm commented 2 years ago

Ok, found a clue:

First, a framesPerBuffer value of zero (== paFramesPerBufferUnspecified) must work, and it currently doesn't with the threaded implementation. So that needs to be fixed. Especially becaues the Portaudio doc recommends use of paFramesPerBufferUnspecified aka zero unless one has good reasons not to.

Second, if we use a framesPerBuffer of, e.g., 64, like in paex_sine.c then the following happens for a stereo 2 channel setup.

The ringbuffer gets initialized to a size of 32768 bytes in pa_linux_pulseaudio.c when calling: PaPulseAudio_BlockingInitRingBuffer(..., stream->outputFrameSize), because stream->outputFrameSize is 8 for 2 channel paFloat 4-Bytes-per-sample. So the ring buffer can hold at most 32768 bytes. However, in the audio processing thread, l_lProcessBytes = pa_stream_writable_size( l_ptrStream->outStream ); reports l_lProcessBytes higher than 32768 Bytes, e.g., 46064 or 35000 etc. The bufferprocessors buffer can never fill up to more than 32768 and so l_lProcessBytes < PaUtil_GetRingBufferReadAvailable( &l_ptrStream->outputRing ) is always false and so PaPulseAudio_writeAudio(l_ptrStream, l_lProcessBytes); does not ever get called, and the result is silence!

I guess it works on your machine buy luck, because your Pulseaudio server / sound hardware requests smaller values of l_lProcessBytes than 32768.

So that calculation logic of bufferprocessor buffer size, and when to call PaPulseAudio_writeAudio and how to handle paFramesPerBufferUnspecified will need to be fixed/improved.

kleinerm commented 2 years ago

Btw.if i turn PaPulseAudio_BlockingInitRingBuffer(..., stream->outputFrameSize) into PaPulseAudio_BlockingInitRingBuffer(..., stream->outputFrameSize * 2) then it works for me, but i guess that is not guaranteed to work on other machines with other audio hw or pulseaudio configurations.

illuusio commented 2 years ago

Thank you for debugging this. So is your FrameSize 0 or big one like 10 000? I have to see how they handle it on ALSA.

kleinerm commented 2 years ago

Normally I specify frameSize in Pa_OpenStream as paFramesPerBufferUnspecified, which is the recommended default setting by the Portaudio docs and defined as 0 in portaudio.h.

illuusio commented 2 years ago

Thank you for sorting this out. I have to test your lib if I just manage to build it :grin:

kleinerm commented 2 years ago

If you want to try my software with your driver, indeed building it is a bit of a hassle as it doesn't have a standard build system. However, essentially nobody does that, as my users are thousands of neuroscientists/psychologists/biologists etc. with generally limited computer skills.

You can get the ready-made software for Debian/Ubuntu from the NeuroDebian project (part of Debian/Science), or as download from us. See http://neuro.debian.net/pkgs/octave-psychtoolbox-3.html for Debian/Ubuntu ppa, or http://psychtoolbox.org/download.html for our own upstream installer.

You'd have to edit the file InitializePsychSound.m though and remove a return; statement in the lower part of that script file, after the IsLinux statement. This to enable use of your pulseaudio backend, otherwise the driver will always go for ALSA directly. The standard demo to try would be BasicSoundOutputDemo which plays until a long enough key press. There are many more demos, for audio capture, full-duplex feedback, precisely timed audio etc., but most would require a one-line tweak or so, to make them choose Pulseaudio instead of ALSA, as ALSA is always chosen when a script declares the need for low latency and high timing precision.

illuusio commented 2 years ago

Now it should work with your code. Sorry took some time to figure out.

illuusio commented 2 years ago

If you want to try my software with your driver, indeed building it is a bit of a hassle as it doesn't have a standard build system. However, essentially nobody does that, as my users are thousands of neuroscientists/psychologists/biologists etc. with generally limited computer skills.

This is bit out of my league but too interesting not to play around.

kleinerm commented 2 years ago

The latest of your branch doesn't work with my code either. Timestamping failure and silence, like before. However, the paFramesPerBufferUnspecified setting now gets mapped to a correct buffer size of 1024 samples, so that's an improvement.

If i again turn PaPulseAudio_BlockingInitRingBuffer(..., stream->outputFrameSize) into PaPulseAudio_BlockingInitRingBuffer(..., stream->outputFrameSize * 2) like before, then i get sound output and some non-zero timestamps, although it is not clear if the timestamps are correct. However, sound plays way to fast and distorted now, like if running on fast-forward on an old tape player.

Wrt. to my software, if you are on Debian/Ubuntu you can use the NeuroDebian ppa packages like explained above and make a one-line change to a file as explained above, to test BasicSoundOutputDemo.m - no need to really rebuild.

illuusio commented 2 years ago

Ok after I merge your PR #9 I'll do the change in allocating RingBuffer bigger as if it's get over 1024 it's too big..

illuusio commented 2 years ago

Output should be greater than it was before. Could you try things out? I have to test Input more that it's more reliable.

kleinerm commented 2 years ago

I just gave it a test run and can happily report that sound output seems to work well now. Good job! Sound input/capture so far doesn't work though, but as you said, this is still a w.i.p.

illuusio commented 2 years ago

Now it should work Input, Output and Duplex modes.. blocking is not working but if this fixes your problem we'll close this bug.

kleinerm commented 2 years ago

Retested on Ubuntu 20.04-LTS.

Good news is that both half-duplex playback and capture now works, whereas yesterday only playback worked.

Bad news:

illuusio commented 2 years ago

Those should now be addressed

kleinerm commented 2 years ago

Nope. Playback and timestamping for playback works. Both capture now hangs - Seems the paCallback never gets called after starting capture.

illuusio commented 2 years ago

Sorry it took so long. Hopefully I finally nail it down and it should work as expected... If you could test latest commit

kleinerm commented 2 years ago

Unfortunately nope, same results as before. Playback and timestamping for playback works. Capture hangs again, and full-Duplex capture + playback doesn't hang anymore, but makes just tick noises or loud hum noises. The SIGFPE floating point exception no longer happens though.

illuusio commented 2 years ago

Ok I took a look at the code and noted that if frame size if paFramesPerBufferUnspecified which is zero then calculation at: https://github.com/illuusio/portaudio/blob/2b2c0e3f48ab1fc9cec9cb9c0b7f13fff8ffaa3d/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1075 initializes zero length input buffer and then it's never calls anything as there is nothing to-do. Could you try to override framesPerBuffer in that line with 512 or something bigger than that. Does it make input work again with paFramesPerBufferUnspecified as I have to tune capture test for it. Strangely it does not return error..

kleinerm commented 2 years ago

That doesn't help, but you are probably on the right track, because if i specify, e.g., framesPerBuffer as 512 or 1024 in Pa_OpenStream() instead of paFramesPerBufferUnspecified, then half-duplex sound capture now works. I can record soudn into memory and then later play it back and the sound sounds correct. I guess paFramesPerBufferUnspecified needs to be mapped to some value earlier in higher level code than there.

Simultaneous capture and playback or full-duplex mode in one of my demos, where i have to rely on timestamps for capture and playback to implement a audio delay effect, works better, but not right:

illuusio commented 2 years ago

Can you share any minimal demo app to play with so I can also see. I assume clicks comes from incorrect calculations of I/O frame size. You don't have enough stuff to input but it requests playing so it fills it with zero and makes click. I'll check it out. Thank you for restless testing!

kleinerm commented 2 years ago

Sorry, I don't have anything like a minimal app. But my whole software is open-source and packaged on Ubuntu and Debian. See https://github.com/illuusio/portaudio/issues/10#issuecomment-1068015591 above for instructions. Version 3.0.18.9 is needed.

I have a gist with two modified files you'll additionally need in your current working directory: https://gist.github.com/kleinerm/629b704cdc2f36e14939a29096217e21

InitializePsychSound.m overrides the standard file, as the standard file does not allow use of the Pulseaudio backend, but always goes through ALSA.

Once you've got Psychtoolbox installed, you can launch octave and use the following demo files:

illuusio commented 1 year ago

Sorry looooooong delay with. I like to get this on next Portaudio so trying to find time to make it work again. Could you see if there is any progress in current branch. If not I'll tune patest_wire test to work as this Psysound thing

kleinerm commented 1 year ago

Retested your latest branch, same result. Pure half-duplex capture or playback works fine. But the simultaneous capture + playback either via two separate half-duplex streams, or one full-duplex streams (see above DelayedSoundFeedbackDemo) is the same. Massive click noises in half-duplex case, wrong delay in full-duplex case - just the distortions are less worse than before.

kleinerm commented 1 year ago

Timestamps in the playback case are also quite variable now, jittering around expected value by +/-15 msecs at a 1024 sample buffer size.

But maybe don't worry too much about this simultaneous capture + playback case. The most basic use cases work ok'ish.

Given that most modern distributions now switch away from Pulseaudio to Pipewire, this might be a bit moot. By the time they will get a new Portaudio release, probably most will have switched. Pipewire afaik exposes both a pulseaudio and a JACK frontend interface. And pro-audio applications would use JACK anyway i assume, so basic Pulseaudio support might be good enough.

illuusio commented 1 year ago

Yes I noted that and started to dig deeper on Pulseaudio API to get timestamps correct and started to use new ways to make XRUNs not to happen.

illuusio commented 1 year ago

Timestamps will be kind of fanny with Pulseaudio always. Now Duplex should not crack anymore if USB/Audio device can handle it.

kleinerm commented 1 year ago

Hi. I left some review comments in your latest commits, with 2 new problems and 1 fix. But apart from that, things seem to be quite improved with your latest iteration. Timestamps seem to be better/reasonable - Don't have the equipment and time around right now for extensive checks, but what it does seems reasonable. And audio quality is good now, for audio capture, audio playback, and full-duplex capture + playback. I'd say progress!

illuusio commented 1 year ago

Nice to hear! I used lot of time for polishing and testing with very not high end equipment and now it should be mostly be there for the masses. Thanks for the comments you made good points with starting and bugs. I'll try to iron them out as they are not very big ones.

illuusio commented 1 year ago

Ok now all reported problems should be fixed.

kleinerm commented 1 year ago

Retested your latest, can confirm your fixes work!

One new issue: https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1195

This doesn't work for "converting" mono audio output to stereo output. Instead the mono audio plays at twice the normal speed at twice the frequency! It appears that if one wants to solve this case, one needs to implement some manual mono->stereo upmixing code oneself, e.g., as done in the Windows/WASAPI backend or some others. The bufferprocessor seems to not be able to do this?

What does work is just removing that statement. Pulseaudio can accept 1 channel mono for playback, at least as tested on my Ubuntu 20.04.6-LTS machine. It just only plays out of the left speaker in a stereo speaker setup. But I think that is much better than playing sound at clearly the wrong speed and pitch.

kleinerm commented 1 year ago

Another thing I realized is that in full-duplex mode, roundtrip latency for capturing a sound from the microphone and then playing it out of the speaker again (an echo) is rather high, about 2 seconds, whereas latency is good if one opens two half-duplex devices, one for input one for output. This seems to be caused by this if-statement, avoiding optimized output settings in full-duplex: https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c#L737

If i turn that into a if (true) to always apply the optimizations, also in full-duplex, then latency in full-duplex is also good and sound still sounds fine? Is this needed to fix some issues, or some leftover from previous experiments?

I also wonder if it would make sense to add PA_STREAM_ADJUST_LATENCY to the stream flags in https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c#L634 as an optimization? This didn't help/do anything on my machine, but maybe it helps somewhere?

I also realized that stream->latency = outputParameters->suggestedLatency etc. is assigned to take a latency hint from the client code, but stream->latency is not actually used anywhere? I guess that could be one way for client code to signal if it wants additional latency optimizations like the ones mentioned here. See: https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1190

Finally, is this line needed? Documentation claims it is a parameter that only applies to recording / input streams, so should be redundant here? https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c#L642

Finally, it seems that the ringbufferSize you calculate in OpenStream starting at https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1104 is not actually used, but a hard-coded value of (65536 * 4) seems to be used instead in https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1125?

Is this intentional? If yes, it could make sense to remove the dead code.

Anyhow, these are just some suggestions from testing/reading/playing with your code yesterday. Not important if they don't make it into the initial implementation / pull-request for upstream portaudio. I think it is now in a rather good shape, at least as far as my testing goes!

illuusio commented 1 year ago

Retested your latest, can confirm your fixes work!

One new issue:

https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1195

This doesn't work for "converting" mono audio output to stereo output. Instead the mono audio plays at twice the normal speed at twice the frequency! It appears that if one wants to solve this case, one needs to implement some manual mono->stereo upmixing code oneself, e.g., as done in the Windows/WASAPI backend or some others. The bufferprocessor seems to not be able to do this?

What does work is just removing that statement. Pulseaudio can accept 1 channel mono for playback, at least as tested on my Ubuntu 20.04.6-LTS machine. It just only plays out of the left speaker in a stereo speaker setup. But I think that is much better than playing sound at clearly the wrong speed and pitch.

Yes I noted that Mono -> Stereo thing too. I've wiped it out couple iterations ago as it seemed to worked but I'll add it back. It's just couple lines of code anyway and with that it works. My bad :-1:

illuusio commented 1 year ago

Another thing I realized is that in full-duplex mode, roundtrip latency for capturing a sound from the microphone and then playing it out of the speaker again (an echo) is rather high, about 2 seconds, whereas latency is good if one opens two half-duplex devices, one for input one for output. This seems to be caused by this if-statement, avoiding optimized output settings in full-duplex:

https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c#L737

If i turn that into a if (true) to always apply the optimizations, also in full-duplex, then latency in full-duplex is also good and sound still sounds fine? Is this needed to fix some issues, or some leftover from previous experiments?

I also wonder if it would make sense to add PA_STREAM_ADJUST_LATENCY to the stream flags in

https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c#L634

as an optimization? This didn't help/do anything on my machine, but maybe it helps somewhere?

From docs If PA_STREAM_INTERPOLATE_TIMING is passed when connecting the stream, pa_stream_get_time() and pa_stream_get_latency() will try to interpolate the current playback time/latency by estimating the number of samples that have been played back by the hardware since the last regular timing update. It just timing update does not affect anything else. PA_STREAM_ADJUST_LATENCY should be applied at least if duplex and it won't hurt even without. I tested with my very small test app and yes fragsize/tlength should be the same or they will be (at least in pipewire) like you described.

I also realized that stream->latency = outputParameters->suggestedLatency etc. is assigned to take a latency hint from the client code, but stream->latency is not actually used anywhere? I guess that could be one way for client code to signal if it wants additional latency optimizations like the ones mentioned here. See:

https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1190

With pulseaudio latency is something that one can't do anything so yes this should be worked out.

Finally, is this line needed? Documentation claims it is a parameter that only applies to recording / input streams, so should be redundant here?

https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c#L642

Finally, it seems that the ringbufferSize you calculate in OpenStream starting at

https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1104

is not actually used, but a hard-coded value of (65536 * 4) seems to be used instead in

https://github.com/illuusio/portaudio/blob/092367395d78327efebb69319473ad3b5a6a35d7/src/hostapi/pulseaudio/pa_linux_pulseaudio.c#L1125 ? Is this intentional? If yes, it could make sense to remove the dead code.

Again some testing code in this iteration. I'll keep the hard-coded it big enough and always there. In future it's can be made smaller as it does not that much space.

Anyhow, these are just some suggestions from testing/reading/playing with your code yesterday. Not important if they don't make it into the initial implementation / pull-request for upstream portaudio. I think it is now in a rather good shape, at least as far as my testing goes!

I think I'll apply most as many other people will tackle them too and file bugs. They are mostly cosmetic and make everyone including me happier.

illuusio commented 1 year ago

I'll fixed and probably created more bugs. This duplex mono thing is something that needs more love than I've though as with patest_mono it works as expect. But need to iron it out when I have some testing device available next week.

kleinerm commented 1 year ago

Retested it with my tests. Looks and sounds all good, except for the unfixed mono->stereo issue.

Maybe you could just skip that assignment for the initial pull request into upstream portaudio, as suggested in my comment https://github.com/illuusio/portaudio/issues/10#issuecomment-1681160378 ? Not perfect, but much better than broken, if somebody wants to use a mono client. Your proper more time consuming solution of properly having to add a lot of mono->stereo upmixing code for the different sample formats could then be a separate PR as you suggested? The WASAPI backind for Windows may have suitable sample code.

As another cleanup to the one i left as a review comment, this line seems to be redundant, as fragsize only affects input, not output according to Pulseaudio docs? Removing it didn't cause any of my tests to fail. https://github.com/illuusio/portaudio/blob/409af01de3357027b3ce7391bce140d6fb393e07/src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c#L643

Anyhow, nice work! I think this is in a good shape for an initial merge.

illuusio commented 1 year ago

I'll tried to make it better. Default 32 get back and remove those un-used stuff. There was this kind of code to make mono -> stereo

if(stream->outputChannelCount == 1)
{
    void *original = stream->outBuffer + length;
    void *stereo = stream->outBuffer;
    memcpy(original, stereo, length);

    for(i = 0; i < length; i += stream->outputFrameSize)
    {
        memcpy(stereo, original, stream->outputFrameSize);
        stereo += stream->outputFrameSize;
        memcpy(stereo, original, stream->outputFrameSize);
        stereo += stream->outputFrameSize;
        original += stream->outputFrameSize;
    }
    length *= 2;
    memcpy(stereo, original, length);
}

I'll try get it back as original code was based version many revision ago.

kleinerm commented 1 year ago

Retested. Looks all good.

illuusio commented 1 year ago

My brain is now confused but stereo issue should be solved and when mono (1 channel) I thinks that then there should be just one output not stereo..

kleinerm commented 1 year ago

Retested. I can confirm that mono-stereo works well enough now. Time to close this issue, I'm happy enough now :).