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

[Georg Chini]

On 21.02.2018 12:50, Raman Shishniou wrote:
> On 02/21/2018 02:24 PM, Georg Chini wrote:
>> On 21.02.2018 12:22, Raman Shishniou wrote:
>>> 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.
>>>
>> Well, your code is not doing the right thing either. When the source gets user
>> suspended, there will be some (trailing) data you read from the pipe. Now you
>> use this data as an indicator, that the source got suspended. When the source
>> gets unsuspended, the first thing you do is post the trailing data that was read
>> when the source was suspended. And only after that you start polling the pipe
>> again
> I can't track the suspend reason in i/o thread right now. It's not copied to
> thread_info in pa_source struct along with state during state changes.
>
> Tanu proposed a patches that will pass pa_suspend_cause_t to SINK/SOURCE_SET_STATE
> handlers and set_state() callbacks. It we add suspend_cause to thread_info too,
> there will be easy way to discard data if we are suspended by user:
>
> if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
>      ... post data ...
>      chunk.length = 0;
> } else if (PA_SUSPEND_APPLICATION is not in thread_info->suspend_cause) {
>      ... discard data ...
>      chunk.length = 0;
> }
>
I see another problem. If, during suspend, a writer connects and
disconnects again, the pipe may be full of old data after we resume.
So I guess we have to read data from the pipe continuously and
discard it while the source is suspended.