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
