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

[Raman Shishniou]

On 02/21/2018 12:13 PM, Raman Shishniou wrote:
> On 02/21/2018 09:39 AM, Georg Chini wrote:
>> On 21.02.2018 06:05, Georg Chini wrote:
>>> On 21.02.2018 05:55, Georg Chini wrote:
>>>> On 20.02.2018 22:34, Raman Shishniou wrote:
>>>>> On 02/20/2018 11:04 PM, Georg Chini wrote:
>>>>>> On 20.02.2018 19:49, Raman Shishniou wrote:
>>>>>>> On 02/20/2018 07:02 PM, Georg Chini wrote:
>>>>>>>> On 20.02.2018 16:38, 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, 212 insertions(+), 67 deletions(-)
>>>>>>>>>
>>>>>>> I think I need post a simple pseudo code of new thread loop because it
>>>>>>> was completely rewritten. There are too many changes in one patch.
>>>>>>> It can be difficult to see the whole picture of new main loop.
>>>>>> Well, I applied the patch and looked at the result. I still don't like the approach.
>>>>>>
>>>>>> I would propose this:
>>>>>>
>>>>>> auto_suspended = false;
>>>>>> revents = 0
>>>>>> events = POLLIN
>>>>>>
>>>>>> for (;;) {
>>>>>>
>>>>>>        /* This is the part that is run when the source is opened
>>>>>>         * or auto suspended
>>>>>>        if (SOURCE_IS_OPENED(source) || auto_suspended) {
>>>>>>
>>>>>>            /* Check if we wake up from user suspend */
>>>>>>            if (corkfd >= 0 && !auto_suspended) {
>>>>>>                 len = 0
>>>>>>                 close pipe for writing
>>>>>>            }
>>>>>>
>>>>>>            /* We received POLLIN or POLLHUP or both */
>>>>>>            if (revents) {
>>>>>>
>>>>>>               /* Read data from pipe */
>>>>>>               len = read data
>>>>>>
>>>>>>               /* Got data, post it */
>>>>>>               if (len > 0) {
>>>>>>                   if (auto_suspend) {
>>>>>>                       send unsuspend message
>>>>>>                       auto_suspend = false
>>>>>>                  }
>>>>>>                  post data
>>>>> We cannot post data here because source still suspended. Sending resume message is not enough
>>>>> to immediately resume the source. We need to wait several poll runs until it will be resumed.
>>>>> (source->thread_info.state changed in this thread, i.e. during poll run). But we will see
>>>>> POLLIN and/or POLLHUP each run if we don't remove pipe fd from polling.
>>>>
>>>> Why do we have to wait? The source will be unsuspended on the next rtpollrun.
>>>> I do not see why we cannot already push data. Or does something get lost?
>>> Why would we receive POLLIN on each run? We read the data from the pipe.
>>> If you think the data should not be posted, you can just skip posting and discard
>>> the data. According to your pseudo-code it is done like tis in your previous patch.
>>
>> I should not write mails before I have woken up completely ... I see what you mean
>> now (and I also see that you do not discard data as I thought). But I still believe you
>> can post the data before the source gets unsuspended. What is the difference if the
>> samples are stored in the source or in the source output? Anyway we are talking
>> about a time frame of (very probably) less than 1 ms between sending the message
>> and receiving it. To ensure that the loop works as expected, auto_suspended should
>> be set/reset in the suspend/unsuspend message and not directly in the thread function.
>> POLLHUP spam cannot happen because corkfd will be opened on the first POLLHUP.
>> POLLIN spam cannot happen when auto_suspend is set/reset from the message
>> handler.
> 
> Not, I can't post it here. The source may not be resumed at all after we send a resume message.
> Not within 1 ms, not within next hour. It can be autosuspended and suspended by user manually
> after it. I that case we read data and should discard it instead of posting (as you propose).
> But that algorithm will post data to suspended source while it suspended by user.
> 
> Also auto_suspended can't be set/reset in suspend/resume message handler because it called from
> main context and accessed from thread context.
> 
> That's why I read data and wait while source will be resumed before posting.
> 

I just looked into pa_source_post() code:

void pa_source_post(pa_source*s, const pa_memchunk *chunk) {
    pa_source_output *o;
    void *state = NULL;

    pa_source_assert_ref(s);
    pa_source_assert_io_context(s);
    pa_assert(PA_SOURCE_IS_LINKED(s->thread_info.state));
    pa_assert(chunk);

    if (s->thread_info.state == PA_SOURCE_SUSPENDED)
        return;

...


There are only 3 valid states of source to post data:
static inline bool PA_SOURCE_IS_LINKED(pa_source_state_t x) {
    return x == PA_SOURCE_RUNNING || x == PA_SOURCE_IDLE || x == PA_SOURCE_SUSPENDED;
}

And if the source is suspended:
if (s->thread_info.state == PA_SOURCE_SUSPENDED)
        return;

If we read some data, send resume and try to post, chunk will be just discarded
in pa_source_post().

So we must to wait source->thread_info.state will be changed to RUNNING or IDLE
before posting any data. And the only way to wait - call some pa_rtpoll_run()
and check the source state to be valid for posting after each call. Again,
we must stop polling pipe while we waiting because we can get endless loop
if source stays suspended for long time after we send a resume message.

I think my algorithm implemented in this patch is the simplest way to achieve this.

--
Raman