On 02/20/2018 07:14 PM, Georg Chini wrote:
> On 20.02.2018 17:08, Raman Shishniou wrote:
>> On 02/20/2018 06:44 PM, Georg Chini wrote:
>>> On 20.02.2018 16:28, Raman Shishniou wrote:
>>>> On 02/20/2018 06:22 PM, Georg Chini wrote:
>>>>> On 20.02.2018 16:13, Georg Chini wrote:
>>>>>> On 20.02.2018 15:47, Raman Shishniou wrote:
>>>>>>> On 02/20/2018 04:55 PM, Georg Chini wrote:
>>>>>>>> On 20.02.2018 14:30, Raman Shishniou wrote:
>>>>>>>>> On 02/20/2018 03:11 PM, Georg Chini wrote:
>>>>>>>>>> On 19.02.2018 16:01, 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, 215 insertions(+), 64 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> for (;;) {
>>>>>>>>>>> int ret;
>>>>>>>>>>> - struct pollfd *pollfd;
>>>>>>>>>>> - pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>>>>>>>>>>> + if (chunk.length) {
>>>>>>>>>>> + /* We have a pending data, let's stop polling pipe.
>>>>>>>>>>> + * Setting up pollfd->events = 0 is not enough to stop
>>>>>>>>>>> + * POLLHUP spam if all writers are closed pipe.
>>>>>>>>>>> + * We need to stop polling pipe completely */
>>>>>>>>>>> + if (rtpoll_item) {
>>>>>>>>>>> + pa_rtpoll_item_free(rtpoll_item);
>>>>>>>>>>> + rtpoll_item = NULL;
>>>>>>>>>>> + }
>>>>>>>>>>> + } else {
>>>>>>>>>>> + /* We have no pending data, let's start polling pipe */
>>>>>>>>>>> + if (rtpoll_item == NULL) {
>>>>>>>>>>> + rtpoll_item = pa_rtpoll_item_new(u->rtpoll, PA_RTPOLL_NEVER, 1);
>>>>>>>>>>> + pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item, NULL);
>>>>>>>>>>> + pollfd->events = POLLIN;
>>>>>>>>>>> + pollfd->fd = u->fd;
>>>>>>>>>>> + }
>>>>>>>>>>> + }
>>>>>>>>>> Why do you need to do that? As in your previous patches you
>>>>>>>>>> open the pipe for writing if all writers are disconnected, which
>>>>>>>>>> should stop POLLHUP's.
>>>>>>>>> This patch tries to be platform-independent.
>>>>>>>>> Unfortunately not all platforms set POLLHUP when a writer closed pipe.
>>>>>>>> Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
>>>>>>>> you know the pipe was closed. If you have POLLIN and l = 0 on your
>>>>>>>> read, you also know the pipe was closed.
>>>>>>>> Then you open your corkfd and stop the POLLHUP/POLLIN.
>>>>>>>> So why do you have to stop polling completely? When the next
>>>>>>>> POLLIN comes, you know that data has been written and POLLHUP
>>>>>>>> can no longer happen.
>>>>>>>>
>>>>>>> If I open pipe for writing before all data was read, i.e. when I see
>>>>>>> POLLHUP - read() will never return 0. Also send suspend message when
>>>>>>> I see POLLHUP is too early. I should open pipe for writing and send
>>>>>>> suspend message when read() return 0.
>>>>>>>
>>>>>>>
>>>>>>>>> Moreover, when POLLHUP was set there may be data in pipe that must be read
>>>>>>>>> before suspend. Linux sets POLLIN in that case. Freebsd and macos sets
>>>>>>>>> POLLIN|POLLHUP even if no any data in pipe, but no writers left. Some platforms
>>>>>>>>> do not set POLLHUP at all. So do read() and check it return 0 must be the only
>>>>>>>>> condition to suspend source and open pipe for writing.
>>>>>>>> Yes, I agree, see above, but I still don't understand why you stop polling.
>>>>>>>>
>>>>>>>>> Also it cover cases when source suspended by user - we read data from pipe,
>>>>>>>>> send resume, but source stays suspended. We must stop polling pipe, but
>>>>>>>>> pollfd->events = 0 is not enough. If the writer make a open-write-close
>>>>>>>>> while source stays suspended we will get endless POLLHUPs
>>>>>>>> I think this could be covered differently. You can set a flag "is_suspended"
>>>>>>>> when you have suspended the source because the writer disconnected.
>>>>>>>> Then you can do something like
>>>>>>>>
>>>>>>>> if (SOURCE_IS_OPENED(source) || is_suspended) {
>>>>>>>> if (!is_suspended && u->corkfd >= 0)
>>>>>>>> close pipe for writing
>>>>>>>>
>>>>>>>> ...
>>>>>>>>
>>>>>>>> } else if (u->corkfd < 0)
>>>>>>>> open pipe for writing
>>>>>>>>
>>>>>>>>
>>>>>>> I need to stop polling to wait for source to be resumed if some bytes was
>>>>>>> read and cannot be posted.
>>>>>>>
>>>>>>> I can get POLLHUP spam if source is suspended, some data is waiting for source
>>>>>>> to be resumed and writer closed pipe.
>>>>>>>
>>>>>>> Example: source suspended by user and by autosuspend, pipe is waiting
>>>>>>> for event (POLLIN), someone opened a pipe and send us data:
>>>>>>>
>>>>>>> First iteration:
>>>>>>>
>>>>>>> 1. poll:
>>>>>>> events = POLLIN
>>>>>>> pa_rtpoll_run()
>>>>>>> revents == POLLIN
>>>>>>> 2. read:
>>>>>>> read() return 100 bytes
>>>>>>> send message to resume source
>>>>>>> 3. post:
>>>>>>> source suspended, keep data in memchunk.
>>>>>>>
>>>>>>> Second iteration, 100 bytes waiting for source to be resumed:
>>>>>>>
>>>>>>> 1. poll:
>>>>>>> events = 0
>>>>>>> pa_rtpoll_run() (processed resume message, but source stays suspended)
>>>>>>> revents == 0
>>>>>>> 2. read:
>>>>>>> some data still pending, do nothing
>>>>>>> 3. post:
>>>>>>> source still suspended, keep pending data
>>>>>>>
>>>>>>> ***writer closed pipe***
>>>>>>>
>>>>>>> Third iteration, POLLHUP spam, 100 bytes still waiting for source to be resumed:
>>>>>> Why would you get POLLHUP spam here with the code I proposed above?
>>>>>> Because the source is suspended by user but not suspended by you,
>>>>>> the code would open the pipe for writing (and close it again as soon
>>>>>> as the user suspend cause is removed). And on the first iteration, you
>>>>>> can discard the data immediately when the source is not opened.
>>>>> The way you are doing it now, the source will in fact keep running, even if it is
>>>>> suspended by the user. One mistake above - you cannot immediately discard
>>>>> the data because at that time you do not know if the source is user suspended
>>>>> so you would have to discard any remaining data at some other point like that:
>>>>>
>>>>> if (SOURCE_IS_OPENED(source) || is_suspended) {
>>>>> if (!is_suspended && u->corkfd >= 0) {
>>>>> close pipe for writing
>>>>> discard remaining data
>>>>> }
>>>>>
>>>>> do all your other processing
>>>>>
>>>>> } else if (u->corkfd < 0)
>>>>> open pipe for writing
>>>>>
>>>> The whole loop can be much easier if I can just discard user data :)
>>>> But why I need to discard any data? I can just wait for source to be
>>>> resumed temporary removing pipe from polling. The source will not keep
>>>> running. It will just wait until it will be resumed.
>>>>
>>>> This is exactly what the original pipe-source is doing.
>>>>
>>> Not executing all that unnecessary code would be better. I never said that
>>> the old code did the right thing.
>>> Regarding discarding the data: If data was read while the source was suspended
>>> by the user, it needs to be discarded on resume, because the the source is not
>>> expected to have some old data.
>>>
>> I can just set pollfd->fd = -1 when no polling is needed instead of freeing
>> the whole rtpoll_item. But I'm not sure if it's supported by any platform
>> except of linux.
>
> I thought about that as well but - like you - am not sure if this
> is supported elsewhere.
>
Actually, freeing and allocation rtpoll_item occurs once per suspend and resume
events. Therefore, it shouldn't be a problem. Suspend and resume events processing
in main thread uses much more resources.
>>
>> I think the old code is doing the right thing. If the source is suspended
>> by user, it was done for a reason. And we don't know if the writer tries to
>> prefill the pipe and than resume the source to avoid underruns or user just
>> stopped the annoying sound. Anyway I think we shouldn't take into account
>> the suspend cause. Even if it automatically suspended.
>>
>> --
> The only suspend cause that would be treated special would be
> the autosuspend cause and you can keep track of it easily. No need
> to know any other reasons, it is enough to know that the source is
> suspended.
I just don't track suspend reason.
Any reason will be processed correctly by the same code.
--
Raman
