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

_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Reply via email to