On 21.02.2018 15:33, Raman Shishniou wrote:
> On 02/21/2018 05:00 PM, Georg Chini wrote:
>> 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.
>>
> Right now yes. This is what original code does.
>
> But if we'll track suspend cause, pending data will be discarded right after
> our unsuspend message will be processed, i.e
>
> {i/o thread} send message to main thread ->
> {main thread} pipe_source_process_msg() will send message to i/o thread ->
> {i/o thread} source_process_msg() will process message from main thread
> and change thread_info.
>
> If PA_SUSPEND_APPLICATION not in suspend_cause but thread still suspended,
> all pending data will be discarded and pipe will be continuously monitored.
> All futher data will be posted or discarded while PA_SUSPEND_APPLICATION not in
> suspend_cause.
>
> So we will stop pipe polling only for short period to wait our unsuspend message will
> be processed.
>
> --
> Raman
Yes, if we track the suspend cause, things will be simpler. But then my
proposal (or rather something very similar) will work as well and I don't
believe it is necessary to stop polling completely.
