On 20.02.2018 17:08, Raman Shishniou wrote:
On 02/20/2018 06:44 PM, Georg Chini wrote:
On 20.02.2018 16:28, Raman Shishniou wrote:
On 02/20/2018 06:22 PM, Georg Chini wrote:
On 20.02.2018 16:13, Georg Chini wrote:
On 20.02.2018 15:47, Raman Shishniou wrote:
On 02/20/2018 04:55 PM, Georg Chini wrote:
On 20.02.2018 14:30, Raman Shishniou wrote:
On 02/20/2018 03:11 PM, Georg Chini wrote:
On 19.02.2018 16:01, Raman Shyshniou wrote:
Currently the pipe-source will remain running even if no
writer is connected and therefore no data is produced.
This patch adds the autosuspend=<bool> option to prevent this.
Source will stay suspended if no writer is connected.
This option is enabled by default.
---
src/modules/module-pipe-source.c | 279
++++++++++++++++++++++++++++++---------
1 file changed, 215 insertions(+), 64 deletions(-)
+
for (;;) {
int ret;
- struct pollfd *pollfd;
- pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
+ if (chunk.length) {
+ /* We have a pending data, let's stop polling pipe.
+ * Setting up pollfd->events = 0 is not enough to stop
+ * POLLHUP spam if all writers are closed pipe.
+ * We need to stop polling pipe completely */
+ if (rtpoll_item) {
+ pa_rtpoll_item_free(rtpoll_item);
+ rtpoll_item = NULL;
+ }
+ } else {
+ /* We have no pending data, let's start polling pipe */
+ if (rtpoll_item == NULL) {
+ rtpoll_item = pa_rtpoll_item_new(u->rtpoll, PA_RTPOLL_NEVER,
1);
+ pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item, NULL);
+ pollfd->events = POLLIN;
+ pollfd->fd = u->fd;
+ }
+ }
Why do you need to do that? As in your previous patches you
open the pipe for writing if all writers are disconnected, which
should stop POLLHUP's.
This patch tries to be platform-independent.
Unfortunately not all platforms set POLLHUP when a writer closed pipe.
Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
you know the pipe was closed. If you have POLLIN and l = 0 on your
read, you also know the pipe was closed.
Then you open your corkfd and stop the POLLHUP/POLLIN.
So why do you have to stop polling completely? When the next
POLLIN comes, you know that data has been written and POLLHUP
can no longer happen.
If I open pipe for writing before all data was read, i.e. when I see
POLLHUP - read() will never return 0. Also send suspend message when
I see POLLHUP is too early. I should open pipe for writing and send
suspend message when read() return 0.
Moreover, when POLLHUP was set there may be data in pipe that must be read
before suspend. Linux sets POLLIN in that case. Freebsd and macos sets
POLLIN|POLLHUP even if no any data in pipe, but no writers left. Some platforms
do not set POLLHUP at all. So do read() and check it return 0 must be the only
condition to suspend source and open pipe for writing.
Yes, I agree, see above, but I still don't understand why you stop polling.
Also it cover cases when source suspended by user - we read data from pipe,
send resume, but source stays suspended. We must stop polling pipe, but
pollfd->events = 0 is not enough. If the writer make a open-write-close
while source stays suspended we will get endless POLLHUPs
I think this could be covered differently. You can set a flag "is_suspended"
when you have suspended the source because the writer disconnected.
Then you can do something like
if (SOURCE_IS_OPENED(source) || is_suspended) {
if (!is_suspended && u->corkfd >= 0)
close pipe for writing
...
} else if (u->corkfd < 0)
open pipe for writing
I need to stop polling to wait for source to be resumed if some bytes was
read and cannot be posted.
I can get POLLHUP spam if source is suspended, some data is waiting for source
to be resumed and writer closed pipe.
Example: source suspended by user and by autosuspend, pipe is waiting
for event (POLLIN), someone opened a pipe and send us data:
First iteration:
1. poll:
events = POLLIN
pa_rtpoll_run()
revents == POLLIN
2. read:
read() return 100 bytes
send message to resume source
3. post:
source suspended, keep data in memchunk.
Second iteration, 100 bytes waiting for source to be resumed:
1. poll:
events = 0
pa_rtpoll_run() (processed resume message, but source stays suspended)
revents == 0
2. read:
some data still pending, do nothing
3. post:
source still suspended, keep pending data
***writer closed pipe***
Third iteration, POLLHUP spam, 100 bytes still waiting for source to be resumed:
Why would you get POLLHUP spam here with the code I proposed above?
Because the source is suspended by user but not suspended by you,
the code would open the pipe for writing (and close it again as soon
as the user suspend cause is removed). And on the first iteration, you
can discard the data immediately when the source is not opened.
The way you are doing it now, the source will in fact keep running, even if it
is
suspended by the user. One mistake above - you cannot immediately discard
the data because at that time you do not know if the source is user suspended
so you would have to discard any remaining data at some other point like that:
if (SOURCE_IS_OPENED(source) || is_suspended) {
if (!is_suspended && u->corkfd >= 0) {
close pipe for writing
discard remaining data
}
do all your other processing
} else if (u->corkfd < 0)
open pipe for writing
The whole loop can be much easier if I can just discard user data :)
But why I need to discard any data? I can just wait for source to be
resumed temporary removing pipe from polling. The source will not keep
running. It will just wait until it will be resumed.
This is exactly what the original pipe-source is doing.
Not executing all that unnecessary code would be better. I never said that
the old code did the right thing.
Regarding discarding the data: If data was read while the source was suspended
by the user, it needs to be discarded on resume, because the the source is not
expected to have some old data.
I can just set pollfd->fd = -1 when no polling is needed instead of freeing
the whole rtpoll_item. But I'm not sure if it's supported by any platform
except of linux.
I thought about that as well but - like you - am not sure if this
is supported elsewhere.
I think the old code is doing the right thing. If the source is suspended
by user, it was done for a reason. And we don't know if the writer tries to
prefill the pipe and than resume the source to avoid underruns or user just
stopped the annoying sound. Anyway I think we shouldn't take into account
the suspend cause. Even if it automatically suspended.
--
The only suspend cause that would be treated special would be
the autosuspend cause and you can keep track of it easily. No need
to know any other reasons, it is enough to know that the source is
suspended.
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
