[pulseaudio-discuss] [PATCH v7] pipe-source: implement autosuspend option

[Georg Chini]

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.


> 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