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

[Georg Chini]

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