[pulseaudio] For source, default timeout should be disconnection ASAP #15314
Conversation
This file contains 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
To avoid weird behaviour, such as pulseaudio server storing audio data between inputstream get/read and then sending the backlog all at once when finally reading, we must disconnect immediately thereafter. Signed-off-by: Gwendal Roulleau <gwendal.roulleau@gmail.com>
dalgwen
force-pushed
the
pulseaudio_source_disconnection_asap
branch
from
d00c182 to
4b1c6cc
Compare
GiviMAD
requested changes
Jul 29, 2023
| @@ -75,7 +75,7 @@ thing-type.config.pulseaudio.source.simpleProtocolSourceFormat.option.s24be = PC | |||
| thing-type.config.pulseaudio.source.simpleProtocolSourceFormat.option.s32le = PCM signed 32-bit little-endian | |||
| thing-type.config.pulseaudio.source.simpleProtocolSourceFormat.option.s32be = PCM signed 32-bit big-endian | |||
| thing-type.config.pulseaudio.source.simpleProtocolSourceIdleTimeout.label = Idle Timeout | |||
| thing-type.config.pulseaudio.source.simpleProtocolSourceIdleTimeout.description = Timeout in ms after which the connection will be closed when no stream is running. This ensures that your speaker is not on all the time and the pulseaudio source can go to idle mode. -1 for no disconnection. | |||
| thing-type.config.pulseaudio.source.simpleProtocolSourceIdleTimeout.description = Timeout in ms after which the connection will be closed when no stream is running. This ensures that your mic is not on all the time and the pulseaudio source can go to idle mode. -1 for no disconnection. 0 for immediate disconnection (recommended value, or expect strange behaviour !). | |||
There was a problem hiding this comment.
I think we can put something more clarifying like "recommended value to avoid audio delay".
There was a problem hiding this comment.
Waiting for the feedback to this comment before merging.
There was a problem hiding this comment.
Indeed, it could be more clarifying. But it is not really an audio delay : it takes audio data from "before" the wanted data.
Maybe : "or expect irrevelant audio data capture"
(not satisfied by this either, I'm open to suggestion)
There was a problem hiding this comment.
Maybe "to avoid problems with buffered audio data" or "to avoid capturing buffered audio"?
There was a problem hiding this comment.
Thank you, feels better.
"to avoid capturing unwanted buffered audio"
Done
Apply revision Signed-off-by: Gwendal Roulleau <gwendal.roulleau@gmail.com>
|
Thank you @lolodomo |
Pshatsillo
pushed a commit
to Pshatsillo/openhab-addons
that referenced
this pull request
Sep 29, 2023
…openhab#15314) * [pulseaudio] For source, default timeout should be disconnection ASAP To avoid weird behaviour, such as pulseaudio server storing audio data between inputstream get/read and then sending the backlog all at once when finally reading, we must disconnect immediately thereafter. --------- Signed-off-by: Gwendal Roulleau <gwendal.roulleau@gmail.com> Co-authored-by: Gwendal Roulleau <gwendal.roulleau@gmail.com>
austvik
pushed a commit
to austvik/openhab-addons
that referenced
this pull request
Mar 27, 2024
…openhab#15314) * [pulseaudio] For source, default timeout should be disconnection ASAP To avoid weird behaviour, such as pulseaudio server storing audio data between inputstream get/read and then sending the backlog all at once when finally reading, we must disconnect immediately thereafter. --------- Signed-off-by: Gwendal Roulleau <gwendal.roulleau@gmail.com> Co-authored-by: Gwendal Roulleau <gwendal.roulleau@gmail.com> Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
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.
Hello all.
When reading from a pulseaudio audio source, a weird behaviour could happen.
Explanation:
There is a (configurable) grace timeout between disconnection. Each read from the socket pushes the timeout forward in time.
This is good for the sink part, as it reduces the need to reconnect socket (and so improve latency), but not so for the source part.
When we get for the first time an inputstream and pass it to the STT service, the binding connects to the socket and returns an appropriate inputstream whose data, hopefully, will immediately be consumed by a TTS service.
When all needed data is collected, the TTS closes the stream from its side (TTS choice, based on VAD or its own max timeout).
The socket is NOT closed. Because there is a pipe in between (usefull to send data to multiple input stream). It is the dedicated pipe that is closed.
The socket remains open during a grace period, allowing a new inputstream to consume data from it faster.
But as the socket remains open, with no inputstream consuming it, audio data is accumulating on the pulseaudio server side.
Then, if openHAB opens a new input stream (because a new vocal command is to be issued) on the socket during the grace period, the pulse audio server will send all accumulated data inbetween to it.
This bug can only happen if openHAB doesn't handle the wake word (because if so, the binding never stops to consume data).
It is the case with the new method "listenAndAnswer".
Solution:
If the socket is disconnected immediately after use, no such behavior happens.
However, I would like to keep the graceperiod as a configuration parameter, for genericity with the sink part. And these weird behaviour could be usefull for whatever use case we cannot imagine right now.
So I suggest to make a new "0" timeout parameter that logically process to disconnect as soon as possible. And make it the default value for source, and let the parameter be an "advanced" parameter to suggest users to not meddle with it without really knowing what they do.
As the method "listenAndAnswer" is new, it could be seen as a regression introduced in 4.0.0 ? On the other side, I don't think many users will use/see this.
Actually, what is the policy for backporting bug fix in release patch branch ?