Fill vchan partially before uncorking stream #207
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marmarek
reviewed
Feb 11, 2024
| pw_log_trace("Uncorking stream after trying to send %" PRIu32 " bytes", | ||
| size); | ||
| uint32_t cmd = QUBES_PA_SINK_UNCORK_CMD; | ||
| if (libvchan_send(stream->impl->stream[PW_DIRECTION_OUTPUT].vchan, &cmd, sizeof(cmd)) != (int)sizeof(cmd)) |
There was a problem hiding this comment.
I had used the wrong vchan for a while. Took a long time to debug. In this case, PW_DIRECTION_OUTPUT is the PipeWire output stream, which corresponds to audio capture, so the code is correct. I will add a comment.
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024021900-4.2&flavor=pull-requests New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024012304-4.2&flavor=update
Failed tests22 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/90505#dependencies 10 fixed
Unstable tests
|
|
This does not help (see test report above, those tests generally correlate with user visible behavior). It's a bit surprising TBH, but also I have attempted similar approach when initially debugging the PW issues and also it had surprisingly negligible (if at all) effect. Maybe there is some implicit uncork somewhere so this change is no-op in practice? |
Maybe there is a bug in pacat-simple-vchan? I’m almost certain that the behavior in this PR is correct. |
DemiMarie
force-pushed
the
fix-problems-early-playback
branch
from
6124273 to
0f121f2
Compare
|
I did not found any implicit uncork, so I added extra logging to see vchan fill level, and it seems not much data is sent before uncorking. The log
This is with with low latency mode enabled. |
|
@marmarek is the stream being corked and then immediately uncorked? There is a race if |
|
No, this happens also directly after starting the pacat process (and also waiting some time before hitting "play", just in case). |
DemiMarie
force-pushed
the
fix-problems-early-playback
branch
from
0f121f2 to
f7e3e4a
Compare
This ensures that pacat-simple-vchan has some data to play and doesn't immediately underrun. There are still A/V sync problems but these are due to not properly reporting latency. Also document some undocumented properties.
No functional change intended.
DemiMarie
force-pushed
the
fix-problems-early-playback
branch
from
f7e3e4a to
7e3a7e4
Compare
|
I collected some more data from logs and made a graph: Legend:
The y-axis is a bit messy, but you can get the idea. When it stabilizes, it's very nice, but initial part is messy - including a lot of underruns during first 4s. I have a bit of trouble interpreting what I see here, but my current guess is some extra buffer that we don't account for. I'll dig deeper into documentation what that is, and how should it be handled. |
|
@marmarek Is this with or without my PR? |
|
With. I tried also tweaking various parameters - setting prefill to 16k in total (delta 8k) helps a bit, but still not fully. Disabling low latency mode helps. This suggests our custom buffer parameters for low latency mode are wrong... I tried some other (raising tlenght, setting prebuf explicitly, setting maxlength etc) but no success. `pactl list sinks` in Dom0 says that sink has 17k (?!) buffer, so prefill of 16k should be good (I don't think this buffer needs to be full to play, in fact full would lead to overruns), but here it looks like there is something else still.
BTW, the limit on prefill looks quite arbitrary, why is it this way? Shouldn't it be vchan buffer size instead (if anything at all)?
|
Looking at the graph, it seems that PulseAudio picks a latency of 0.2 seconds! This is extremely wrong — the latency should actually be around 23 milliseconds, or about 11.6% of the actual value. The result is that PulseAudio keeps draining the buffer way more quickly than the agent expects, causing underruns.
The prefill value used internally is There are two reasons for this:
|
|
The max latency is I think due to not setting maxlength, but that's separate from underruns. See underruns happen only early, and stop before latency starts to climb.
Maybe I misunderstood delta parameter, what should I set to prefill with more data? For example to initially fill the whole vchan?
As for script, not really, it's `timeout 1m yes aaa | paplay --raw` in the VM and watch for underruns in pacat log. You can also `parecord file` in Dom0 (after setting default recording to monitor stream), with this simple "sound" you can easily check for underruns by looking for series of zeroes (hexdump will coalesce identical lines, so that's easy to see).
I have a script to make the graph, but that requires extra logging in pacat, and also I don't have it with me right now.
|
|
Some tests on my system:
Conclusion: PulseAudio daemon bug. Switching to PipeWire in dom0 is the obvious fix. |
|
With PipeWire in dom0, the prebuf feature seems to be working correctly (at least for me), so I think this PR isn't needed after all. But also, looking at the graph above, the vchan fill level stabilizes at about 12KiB, which doesn't match the configured 8KiB. Maybe it should be set to 4KiB by default instead? |
|
I've scheduled openQA run without this PR, it will run on both real hw and nested virt, lets wait for results with the decision. |
|
@marmarek is that with PipeWire in dom0? |
That's the intention, if QubesOS/qubes-core-admin-linux#143 works. |
|
Closing as per this comment from @marmarek. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This ensures that pacat-simple-vchan has some data to play and doesn't immediately underrun. There are still A/V sync problems but these are due to not properly reporting latency.