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

[Raman Shishniou]

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:

1. poll:
      events = 0
      pa_rtpoll_run()
      revents == POLLHUP
2. read:
      some data still pending, do nothing
3. post:
      source still suspended, keep pending data

--
Raman