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;
}
--
Raman
