Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 22.02.2018 09:49, Tanu Kaskinen wrote: On Thu, 2018-02-22 at 09:26 +0100, Georg Chini wrote: On 21.02.2018 21:02, Georg Chini wrote: On 21.02.2018 20:15, Raman Shishniou wrote: On 02/21/2018 09:41 PM, Raman Shuishniou wrote: 21.02.2018 20:07, Georg Chini пишет: Maybe you misunderstood me. What I mean, is that the pipe can be opened for writing as long as we are suspended. So it open when we see that the source is suspended or when we auto suspend. Close it as soon as the source gets unsuspended. This will avoid POLLHUP during suspend completely. While auto suspended, we additionally have to listen for POLLIN. This way we can only get POLLHUP (or POLLIN with no data) when the source is running. I think I understand. We need to keep our writer opened while transition from autosuspended to opened state and just set events = 0. Exactly. I was also thinking about how to decide if data can be discarded or needs to be posted. On resume, the SET_STATE handler could set a just_unsuspended flag and when we know auto suspend is the only suspend cause, we can additionally set a resume_from_auto_suspend flag when we send the unsuspend message. In the thread function we could then check if just_unsuspended && resume_from_autosuspend -> post pending data, reset flags just_unsuspended && !resume_from_auto_suspend -> discard data, reset flags !just_unsuspended -> post data But that's only an idea. It's even simpler, because the old and new suspend cause are available in the SET_STATE handler (or in the set_state_in_io_thread() callback). So there we can do: if (old_cause == AUTO_SUSPEND && new_cause == 0) -> resume_from_auto_suspend = true else if (old_caue != 0 && new_cause == 0) -> resume_from_user_suspend = true BTW, maybe we should have a new suspend cause for that. Currently, the best suspend cause we could use is PA_SUSPEND_UNAVAILABLE. In the future however, it is planned, that streams get rescued when the suspend cause is PA_SUSPEND_UNAVAILABLE and I guess that is not what you want in your case. IIRC, we agreed that the stream rescuing will be done based on port availability, so the suspend state and cause will not be considered. OK, my understanding was it would work like this: No port -> set SUSPEND_UNAVAILABLE If SUSPEND_UNAVAILABLE gets set -> rescue stream. This would make it somewhat more generic and not alsa specific. But I am OK with using the port availability directly. FWIW, I'm OK with using any of UNAVAILABLE, APPLICATION or some new suspend cause in the pipe source. I'd like to some day change the suspend system that the causes are identified by strings (in a hashmap) instead of enumeration values so that we don't have to pretend having some generic suspend causes when in reality each of them tends to be very specific to a particular piece of code. I'd say the best fit in that case is PA_SUSPEND_UNAVAILABLE. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On Thu, 2018-02-22 at 09:26 +0100, Georg Chini wrote: > On 21.02.2018 21:02, Georg Chini wrote: > > On 21.02.2018 20:15, Raman Shishniou wrote: > > > On 02/21/2018 09:41 PM, Raman Shuishniou wrote: > > > > 21.02.2018 20:07, Georg Chini пишет: > > > > > Maybe you misunderstood me. What I mean, is that the pipe can be > > > > > opened for writing as long as we are suspended. So it open when > > > > > we see that the source is suspended or when we auto suspend. Close > > > > > it as soon as the source gets unsuspended. This will avoid POLLHUP > > > > > during suspend completely. While auto suspended, we additionally have > > > > > to listen for POLLIN. > > > > > This way we can only get POLLHUP (or POLLIN with no data) when the > > > > > source is running. > > > > > > > > > > > > > I think I understand. We need to keep our writer opened while > > > > transition > > > > from autosuspended to opened state and just set events = 0. > > > > Exactly. I was also thinking about how to decide if data can be discarded > > or needs to be posted. On resume, the SET_STATE handler could set a > > just_unsuspended flag and when we know auto suspend is the only > > suspend cause, we can additionally set a resume_from_auto_suspend > > flag when we send the unsuspend message. In the thread function we > > could then check if > > > > just_unsuspended && resume_from_autosuspend -> post pending data, > > reset flags > > just_unsuspended && !resume_from_auto_suspend -> discard data, reset > > flags > > !just_unsuspended -> post data > > > > But that's only an idea. > > It's even simpler, because the old and new suspend cause are available > in the > SET_STATE handler (or in the set_state_in_io_thread() callback). So > there we can > do: > if (old_cause == AUTO_SUSPEND && new_cause == 0) -> > resume_from_auto_suspend = true > else if (old_caue != 0 && new_cause == 0) -> resume_from_user_suspend = true > > BTW, maybe we should have a new suspend cause for that. Currently, > the best suspend cause we could use is PA_SUSPEND_UNAVAILABLE. > In the future however, it is planned, that streams get rescued when > the suspend cause is PA_SUSPEND_UNAVAILABLE and I guess that is > not what you want in your case. IIRC, we agreed that the stream rescuing will be done based on port availability, so the suspend state and cause will not be considered. FWIW, I'm OK with using any of UNAVAILABLE, APPLICATION or some new suspend cause in the pipe source. I'd like to some day change the suspend system that the causes are identified by strings (in a hashmap) instead of enumeration values so that we don't have to pretend having some generic suspend causes when in reality each of them tends to be very specific to a particular piece of code. -- Tanu https://liberapay.com/tanuk https://www.patreon.com/tanuk ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 21.02.2018 21:02, Georg Chini wrote: On 21.02.2018 20:15, Raman Shishniou wrote: On 02/21/2018 09:41 PM, Raman Shuishniou wrote: 21.02.2018 20:07, Georg Chini пишет: Maybe you misunderstood me. What I mean, is that the pipe can be opened for writing as long as we are suspended. So it open when we see that the source is suspended or when we auto suspend. Close it as soon as the source gets unsuspended. This will avoid POLLHUP during suspend completely. While auto suspended, we additionally have to listen for POLLIN. This way we can only get POLLHUP (or POLLIN with no data) when the source is running. I think I understand. We need to keep our writer opened while transition from autosuspended to opened state and just set events = 0. Exactly. I was also thinking about how to decide if data can be discarded or needs to be posted. On resume, the SET_STATE handler could set a just_unsuspended flag and when we know auto suspend is the only suspend cause, we can additionally set a resume_from_auto_suspend flag when we send the unsuspend message. In the thread function we could then check if just_unsuspended && resume_from_autosuspend -> post pending data, reset flags just_unsuspended && !resume_from_auto_suspend -> discard data, reset flags !just_unsuspended -> post data But that's only an idea. It's even simpler, because the old and new suspend cause are available in the SET_STATE handler (or in the set_state_in_io_thread() callback). So there we can do: if (old_cause == AUTO_SUSPEND && new_cause == 0) -> resume_from_auto_suspend = true else if (old_caue != 0 && new_cause == 0) -> resume_from_user_suspend = true BTW, maybe we should have a new suspend cause for that. Currently, the best suspend cause we could use is PA_SUSPEND_UNAVAILABLE. In the future however, it is planned, that streams get rescued when the suspend cause is PA_SUSPEND_UNAVAILABLE and I guess that is not what you want in your case. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 02/21/2018 09:41 PM, Raman Shuishniou wrote: > 21.02.2018 20:07, Georg Chini пишет: >> Maybe you misunderstood me. What I mean, is that the pipe can be >> opened for writing as long as we are suspended. So it open when >> we see that the source is suspended or when we auto suspend. Close >> it as soon as the source gets unsuspended. This will avoid POLLHUP >> during suspend completely. While auto suspended, we additionally have >> to listen for POLLIN. >> This way we can only get POLLHUP (or POLLIN with no data) when the >> source is running. >> > > I think I understand. We need to keep our writer opened while transition > from autosuspended to opened state and just set events = 0. > > But what I don't understand - why you so hate the freeing and allocating > rtpoll_item during this transition? > > I'll try ro rewrite the patch (again) while we waiting for Tanu to apply > his patches. > Also, did I still need to make this behaviour optional? -- Raman ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 21.02.2018 20:15, Raman Shishniou wrote: On 02/21/2018 09:41 PM, Raman Shuishniou wrote: 21.02.2018 20:07, Georg Chini пишет: Maybe you misunderstood me. What I mean, is that the pipe can be opened for writing as long as we are suspended. So it open when we see that the source is suspended or when we auto suspend. Close it as soon as the source gets unsuspended. This will avoid POLLHUP during suspend completely. While auto suspended, we additionally have to listen for POLLIN. This way we can only get POLLHUP (or POLLIN with no data) when the source is running. I think I understand. We need to keep our writer opened while transition from autosuspended to opened state and just set events = 0. Exactly. I was also thinking about how to decide if data can be discarded or needs to be posted. On resume, the SET_STATE handler could set a just_unsuspended flag and when we know auto suspend is the only suspend cause, we can additionally set a resume_from_auto_suspend flag when we send the unsuspend message. In the thread function we could then check if just_unsuspended && resume_from_autosuspend -> post pending data, reset flags just_unsuspended && !resume_from_auto_suspend -> discard data, reset flags !just_unsuspended -> post data But that's only an idea. But what I don't understand - why you so hate the freeing and allocating rtpoll_item during this transition? I don't hate it. However I think it looks weird and none of the other thread functions does something like this. If somebody looks at the code in a year or so, it will be hard to figure out why it was done that way. This is a general thing - using similar patterns where possible, adding comments and sometimes even writing more code just for clarity make the the software better maintainable. I'll try ro rewrite the patch (again) while we waiting for Tanu to apply his patches. Sorry. Meanwhile I have done this often enough with my patches when Tanu disagreed. I know it's really annoying but in the end those discussions improve the quality of the code because we will end up with something we both can live with (and that will probably really do what was intended). Also, did I still need to make this behaviour optional? Yes, I think it should still be optional, but it is OK to make it default. The reason is that we keep backwards compatibility wherever possible. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
21.02.2018 20:07, Georg Chini пишет: Maybe you misunderstood me. What I mean, is that the pipe can be opened for writing as long as we are suspended. So it open when we see that the source is suspended or when we auto suspend. Close it as soon as the source gets unsuspended. This will avoid POLLHUP during suspend completely. While auto suspended, we additionally have to listen for POLLIN. This way we can only get POLLHUP (or POLLIN with no data) when the source is running. I think I understand. We need to keep our writer opened while transition from autosuspended to opened state and just set events = 0. But what I don't understand - why you so hate the freeing and allocating rtpoll_item during this transition? I'll try ro rewrite the patch (again) while we waiting for Tanu to apply his patches. -- Raman ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 21.02.2018 17:09, Raman Shishniou wrote:
On 02/21/2018 06:43 PM, Georg Chini wrote:
On 21.02.2018 16:15, Raman Shishniou wrote:
On 02/21/2018 05:59 PM, Georg Chini wrote:
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= 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(-)
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.
No, it does not. It stops reading the pipe, it sets events = 0 on suspend.
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.
We still can get POLLHUP spam while waiting for couple of milliseconds:
Writer open() pipe, write() some data and close() it immediately.
We got data, close corkfd, send unsuspend message, set events = 0, but will
get POLLHUP every poll run until our unsuspend message will be processed.
My proposal opens the corkfd again and keeps it open until the
source is running. So no POLLHUP spam.
We will never receive 0 from pipe if we open corkfd as soon as we see POLLHUP.
Maybe you misunderstood me. What I mean, is that the pipe can be
opened for writing as long as we are suspended. So it open when
we see that the source is suspended or when we auto suspend. Close
it as soon as the source gets unsuspended. This will avoid POLLHUP
during suspend completely. While auto suspended, we additionally have
to listen for POLLIN.
This way we can only get POLLHUP (or POLLIN with no data) when the
source is running.
Moreover, some platforms do not send POLLHUP at all. We c
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 02/21/2018 06:43 PM, Georg Chini wrote:
> On 21.02.2018 16:15, Raman Shishniou wrote:
>> On 02/21/2018 05:59 PM, Georg Chini wrote:
>>> 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= 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(-)
>
>>>
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.
>
>
> No, it does not. It stops reading the pipe, it sets events = 0 on suspend.
>
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.
>>>
>> We still can get POLLHUP spam while waiting for couple of milliseconds:
>>
>> Writer open() pipe, write() some data and close() it immediately.
>>
>> We got data, close corkfd, send unsuspen
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 21.02.2018 16:15, Raman Shishniou wrote:
On 02/21/2018 05:59 PM, Georg Chini wrote:
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= 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(-)
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.
No, it does not. It stops reading the pipe, it sets events = 0 on suspend.
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.
We still can get POLLHUP spam while waiting for couple of milliseconds:
Writer open() pipe, write() some data and close() it immediately.
We got data, close corkfd, send unsuspend message, set events = 0, but will
get POLLHUP every poll run until our unsuspend message will be processed.
My proposal opens the corkfd again and keeps it open until the
source is running. So no POLLHUP spam.
It will be like kernel's spin lock - endless loop for short undefined
period of time (1us - 1ms) - just waste of CPU.
The way we completely remove pipe fd from polling have to be
simpler - without freeing and allocating memory, but this is the only
way current polling implementation gives to us.
I still do not see it is necessary and I think there is no need that the
thread function is so different from all other thread functions.
It helps a lot when the code is consistent. Also your thread loop is
difficult to understand and uses indirect hints (like the length of
some data to indicate that the source is suspended or a file
descriptor is set to indicate auto
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 02/21/2018 05:59 PM, Georg Chini wrote:
> 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= 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,
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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 s
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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 *
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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 som
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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 inlin
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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.
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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?
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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.
>
> /* Got POLLHUP or POLLIN without data, writer disconnected */
> } else if (len = 0) {
> send suspend message
> auto_suspend = true
> }
>
> /* else Handle error */
> else
> handle error
>
> events = POLLIN
> }
>
> /* This is the part that is run, when the source is
> * suspended by user */
> } else {
>
>if (corkfd < 0)
> open pipe for writing
>events = 0
> }
>
> /* This is run always */
> run rtpoll
> ... check errors ...
> }
>
> I think this is more consistent with how the other thread functions are
> written and also has a clearer structure.
>
>>
>> pollfd = NULL;
>> rtpoll_item = NULL;
>> chunk.length = 0; // length of pending data in chunk.memblock
>> chunk.memblock = pa_memblock_new(); // always allocated
>>
>> for (;;) {
>>
>> /**
>> * run rtpoll
>> */
>> if (chunk.length > 0) {
>> /* we have a pending data */
>> if (rtpoll_item) {
>> /* stop polling pipe
>> * the only way to be here:
>> * source was just suspended */
>> ... free rtpoll_item ...
>> rtpoll_item = NULL;
>> }
>> } else {
>> /* we have no pending data */
>> if (rtpoll_item == NULL) {
>> /* start polling pipe
>> * the only way to be here:
>> * source was just resumed */
>> ... allocate rtpoll_item, get pollfd ...
>> pollfd->events = POLLIN;
>> pollfd->fd = u->fd;
>> }
>> }
>>
>> pa_rtpoll_run()
>>
>> ... check errors ...
>>
>> if (rtpoll_item) {
>> /* we polled pipe */
>> ... refresh pollfd ...
>> } else
>> /* we are waiting for source state changes */
>> pollfd = NULL;
>>
>> /**
>> * Read data
>> */
>> if (pollfd && pollfd->revents) {
>>
>> ... read data from pipe ...
>>
>> if (len > 0) {
>> chunk.length = len;
>>
>> /* source is autosuspended? */
>> if (u->corkfd >= 0) {
>> ... send resume message ...
>> close(u->corkfd);
>> u->corkfd = -1;
>> }
>> }
>>
>> if (len == 0) {
>> /* sourece not autosuspended? */
>> if (u->corkfd < 0) {
>> ... send suspend message ...
>> u->corkfd = open(pipe, O_WRONLY);
>> }
>> }
>>
>> if (len < 0) {
>> ... check read error ...
>>
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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
>
> /* Got POLLHUP or POLLIN without data, writer disconnected */
> } else if (len = 0) {
> send suspend message
> auto_suspend = true
> }
>
> /* else Handle error */
> else
> handle error
>
> events = POLLIN
> }
>
> /* This is the part that is run, when the source is
> * suspended by user */
> } else {
>
>if (corkfd < 0)
> open pipe for writing
>events = 0
> }
>
> /* This is run always */
> run rtpoll
> ... check errors ...
> }
>
> I think this is more consistent with how the other thread functions are
> written and also has a clearer structure.
>
Ok. I'll try to do something like this.
>>
>> pollfd = NULL;
>> rtpoll_item = NULL;
>> chunk.length = 0; // length of pending data in chunk.memblock
>> chunk.memblock = pa_memblock_new(); // always allocated
>>
>> for (;;) {
>>
>> /**
>> * run rtpoll
>> */
>> if (chunk.length > 0) {
>> /* we have a pending data */
>> if (rtpoll_item) {
>> /* stop polling pipe
>> * the only way to be here:
>> * source was just suspended */
>> ... free rtpoll_item ...
>> rtpoll_item = NULL;
>> }
>> } else {
>> /* we have no pending data */
>> if (rtpoll_item == NULL) {
>> /* start polling pipe
>> * the only way to be here:
>> * source was just resumed */
>> ... allocate rtpoll_item, get pollfd ...
>> pollfd->events = POLLIN;
>> pollfd->fd = u->fd;
>> }
>> }
>>
>> pa_rtpoll_run()
>>
>> ... check errors ...
>>
>> if (rtpoll_item) {
>> /* we polled pipe */
>> ... refresh pollfd ...
>> } else
>> /* we are waiting for source state changes */
>> pollfd = NULL;
>>
>> /**
>> * Read data
>> */
>> if (pollfd && pollfd->revents) {
>>
>> ... read data from pipe ...
>>
>> if (len > 0) {
>> chunk.length = len;
>>
>> /* source is autosuspended? */
>> if (u->corkfd >= 0) {
>> ... send resume message ...
>> close(u->corkfd);
>> u->corkfd = -1;
>> }
>> }
>>
>> if (len == 0) {
>> /* sourece not autosuspended? */
>> if (u->corkfd < 0) {
>> ... send suspend message ...
>> u->corkfd = open(pipe, O_WRONLY);
>> }
>> }
>>
>> if (len < 0) {
>> ... check read error ...
>> }
>> }
>>
>> /**
>> * Post data
>> */
>> if (source is opened) {
>
> After an auto-unsuspend, the result of the above comparison is undefined.
> It may be that the main thread already processed the unsuspend message,
> maybe not.
The a
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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
/* Got POLLHUP or POLLIN without data, writer disconnected */
} else if (len = 0) {
send suspend message
auto_suspend = true
}
/* else Handle error */
else
handle error
events = POLLIN
}
/* This is the part that is run, when the source is
* suspended by user */
} else {
if (corkfd < 0)
open pipe for writing
events = 0
}
/* This is run always */
run rtpoll
... check errors ...
}
I think this is more consistent with how the other thread functions are
written and also has a clearer structure.
pollfd = NULL;
rtpoll_item = NULL;
chunk.length = 0; // length of pending data in chunk.memblock
chunk.memblock = pa_memblock_new(); // always allocated
for (;;) {
/**
* run rtpoll
*/
if (chunk.length > 0) {
/* we have a pending data */
if (rtpoll_item) {
/* stop polling pipe
* the only way to be here:
* source was just suspended */
... free rtpoll_item ...
rtpoll_item = NULL;
}
} else {
/* we have no pending data */
if (rtpoll_item == NULL) {
/* start polling pipe
* the only way to be here:
* source was just resumed */
... allocate rtpoll_item, get pollfd ...
pollfd->events = POLLIN;
pollfd->fd = u->fd;
}
}
pa_rtpoll_run()
... check errors ...
if (rtpoll_item) {
/* we polled pipe */
... refresh pollfd ...
} else
/* we are waiting for source state changes */
pollfd = NULL;
/**
* Read data
*/
if (pollfd && pollfd->revents) {
... read data from pipe ...
if (len > 0) {
chunk.length = len;
/* source is autosuspended? */
if (u->corkfd >= 0) {
... send resume message ...
close(u->corkfd);
u->corkfd = -1;
}
}
if (len == 0) {
/* sourece not autosuspended? */
if (u->corkfd < 0) {
... send suspend message ...
u->corkfd = open(pipe, O_WRONLY);
}
}
if (len < 0) {
... check read error ...
}
}
/**
* Post data
*/
if (source is opened) {
After an auto-unsuspend, the result of the above comparison is undefined.
It may be that the main thread already processed the unsuspend message,
maybe not.
... post data ...
unref(chunk.memblock);
chunk.memblock = pa_memblock_new();
chunk.length = 0;
}
/* chunk.length can be greater than 0 only if we are suspended right now
* we need to stop polling pipe and wait while state will be changed to
RUNNING or IDLE
* to post pending data */
If we are user suspended right now, the data should never be posted.
Data that is read
while we are user-suspended should be discarded. Data that arrives while
w
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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.
pollfd = NULL;
rtpoll_item = NULL;
chunk.length = 0; // length of pending data in chunk.memblock
chunk.memblock = pa_memblock_new(); // always allocated
for (;;) {
/**
* run rtpoll
*/
if (chunk.length > 0) {
/* we have a pending data */
if (rtpoll_item) {
/* stop polling pipe
* the only way to be here:
* source was just suspended */
... free rtpoll_item ...
rtpoll_item = NULL;
}
} else {
/* we have no pending data */
if (rtpoll_item == NULL) {
/* start polling pipe
* the only way to be here:
* source was just resumed */
... allocate rtpoll_item, get pollfd ...
pollfd->events = POLLIN;
pollfd->fd = u->fd;
}
}
pa_rtpoll_run()
... check errors ...
if (rtpoll_item) {
/* we polled pipe */
... refresh pollfd ...
} else
/* we are waiting for source state changes */
pollfd = NULL;
/**
* Read data
*/
if (pollfd && pollfd->revents) {
... read data from pipe ...
if (len > 0) {
chunk.length = len;
/* source is autosuspended? */
if (u->corkfd >= 0) {
... send resume message ...
close(u->corkfd);
u->corkfd = -1;
}
}
if (len == 0) {
/* sourece not autosuspended? */
if (u->corkfd < 0) {
... send suspend message ...
u->corkfd = open(pipe, O_WRONLY);
}
}
if (len < 0) {
... check read error ...
}
}
/**
* Post data
*/
if (source is opened) {
... post data ...
unref(chunk.memblock);
chunk.memblock = pa_memblock_new();
chunk.length = 0;
}
/* chunk.length can be greater than 0 only if we are suspended right now
* we need to stop polling pipe and wait while state will be changed to
RUNNING or IDLE
* to post pending data */
}
I can convert all changes to patch series if you want.
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
On 02/20/2018 07:19 PM, Raman Shishniou wrote:
>>> +if (chunk.length) {
If the source is running or idle, the chunk.length always will be 0.
if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
...
chunk.length = 0;
}
guaranteed that.
>>> +/* 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) {
Here rtpoll_item will be freed only once after source was suspended.
It will stay free until the source will be resumed.
>>> +pa_rtpoll_item_free(rtpoll_item);
>>> +rtpoll_item = NULL;
>>> +}
>>> +} else {
>>> +/* We have no pending data, let's start polling pipe */
>>> +if (rtpoll_item == NULL) {
Here rtpoll_item will be allocated only once after source was resumed and
processed all pending data.
>>> +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;
>>> +}
>>> +}
>>
>> Your code will allocate/deallocate the rtpoll_item on each
>> iteration. This is unnecessary and CPU intensive (I was told).
>> I would still prefer my approach and I only see disadvantages
>> with your way.
>>
>
> No, it's not like that.
>
> rtpoll_item will be allocated:
> - On thread start
> - We just processed all pending data, i.e. source was just resumed.
>
> rtpoll_item will be freed:
> - We were got any data from pipe, but source was just suspended.
>
> So there will be only one free per suspend and one allocate per resume.
>
>
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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(-)
>>
>> diff --git a/src/modules/module-pipe-source.c
>> b/src/modules/module-pipe-source.c
>> index f8284c1..359cdbf 100644
>> --- a/src/modules/module-pipe-source.c
>> +++ b/src/modules/module-pipe-source.c
>> @@ -33,6 +33,7 @@
>> #include
>> #endif
>> +#include
>> #include
>> #include
>> @@ -57,11 +58,24 @@ PA_MODULE_USAGE(
>> "format= "
>> "rate= "
>> "channels= "
>> -"channel_map=");
>> +"channel_map= "
>> +"autosuspend=");
>> #define DEFAULT_FILE_NAME "/tmp/music.input"
>> #define DEFAULT_SOURCE_NAME "fifo_input"
>> +struct pipe_source_msg {
>> +pa_msgobject parent;
>> +};
>> +
>> +typedef struct pipe_source_msg pipe_source_msg;
>> +PA_DEFINE_PRIVATE_CLASS(pipe_source_msg, pa_msgobject);
>> +
>> +enum {
>> +PIPE_SOURCE_SUSPEND,
>> +PIPE_SOURCE_RESUME
>> +};
>> +
>> struct userdata {
>> pa_core *core;
>> pa_module *module;
>> @@ -71,12 +85,14 @@ struct userdata {
>> pa_thread_mq thread_mq;
>> pa_rtpoll *rtpoll;
>> +pipe_source_msg *msg;
>> +pa_usec_t timestamp;
>> +bool autosuspend;
>> +size_t pipe_size;
>> +
>> char *filename;
>> +int corkfd;
>> int fd;
>> -
>> -pa_memchunk memchunk;
>> -
>> -pa_rtpoll_item *rtpoll_item;
>> };
>> static const char* const valid_modargs[] = {
>> @@ -87,9 +103,41 @@ static const char* const valid_modargs[] = {
>> "rate",
>> "channels",
>> "channel_map",
>> +"autosuspend",
>> NULL
>> };
>> +/* Called from main context */
>> +static int pipe_source_process_msg(
>> +pa_msgobject *o,
>> +int code,
>> +void *data,
>> +int64_t offset,
>> +pa_memchunk *chunk) {
>> +
>> +struct userdata *u = (struct userdata *) data;
>> +
>> +pa_assert(u);
>> +
>> +switch (code) {
>> +case PIPE_SOURCE_SUSPEND:
>> +pa_log_debug("Suspending source %s because no writers left",
>> u->source->name);
>> +pa_source_suspend(u->source, true, PA_SUSPEND_APPLICATION);
>> +break;
>> +
>> +case PIPE_SOURCE_RESUME:
>> +pa_log_debug("Resuming source %s", u->source->name);
>> +pa_source_suspend(u->source, false, PA_SUSPEND_APPLICATION);
>> +break;
>> +
>> +default:
>> +pa_assert_not_reached();
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/* Called from thread context */
>> static int source_process_msg(
>> pa_msgobject *o,
>> int code,
>> @@ -101,17 +149,19 @@ static int source_process_msg(
>> switch (code) {
>> -case PA_SOURCE_MESSAGE_GET_LATENCY: {
>> -size_t n = 0;
>> +case PA_SOURCE_MESSAGE_SET_STATE:
>> -#ifdef FIONREAD
>> -int l;
>> +if (u->source->thread_info.state == PA_SOURCE_SUSPENDED ||
>> u->source->thread_info.state == PA_SOURCE_INIT) {
>> +if (PA_SOURCE_IS_OPENED(PA_PTR_TO_UINT(data)))
>> +u->timestamp = pa_rtclock_now();
>> +}
>> -if (ioctl(u->fd, FIONREAD, &l) >= 0 && l > 0)
>> -n = (size_t) l;
>> -#endif
>> +break;
>> +
>> +case PA_SOURCE_MESSAGE_GET_LATENCY: {
>> +
>> +*((int64_t*) data) = PA_CLIP_SUB((int64_t)pa_rtclock_now(),
>> (int64_t)u->timestamp);
>> -*((int64_t*) data) = pa_bytes_to_usec(n,
>> &u->source->sample_spec);
>> return 0;
>> }
>> }
>> @@ -121,7 +171,11 @@ static int source_process_msg(
>> static void thread_func(void *userdata) {
>> struct userdata *u = userdata;
>> +pa_rtpoll_item *rtpoll_item;
>> +struct pollfd *pollfd;
>> +pa_memchunk chunk;
>> int read_type = 0;
>> +size_t fs;
>> pa_assert(u);
>> @@ -129,68 +183,140 @@ static void thread_func(void *userdata) {
>> pa_thread_mq_install(&u->thread_mq);
>> +fs = pa_frame_size(&u->source->sample_spec);
>> +
>> +pa_memchunk_reset(&chunk);
>> +chunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
>> +
>> +rtpoll_item = NULL;
>> +pollfd = NULL;
>> +
>> +u->timestamp = pa_rtclock_now();
>> +
>> +/* Close our writer here to suspend source if no writers left */
>> +pa_assert_se(pa_close(u->corkfd) == 0);
>> +u->corkfd = -1;
>> +
>> for (;;) {
>> int ret;
>> -struct pollfd *pollfd;
>> -pollfd = pa_rtpoll_ite
Re: [pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option
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= 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(-)
diff --git a/src/modules/module-pipe-source.c b/src/modules/module-pipe-source.c
index f8284c1..359cdbf 100644
--- a/src/modules/module-pipe-source.c
+++ b/src/modules/module-pipe-source.c
@@ -33,6 +33,7 @@
#include
#endif
+#include
#include
#include
@@ -57,11 +58,24 @@ PA_MODULE_USAGE(
"format= "
"rate= "
"channels= "
-"channel_map=");
+"channel_map= "
+"autosuspend=");
#define DEFAULT_FILE_NAME "/tmp/music.input"
#define DEFAULT_SOURCE_NAME "fifo_input"
+struct pipe_source_msg {
+pa_msgobject parent;
+};
+
+typedef struct pipe_source_msg pipe_source_msg;
+PA_DEFINE_PRIVATE_CLASS(pipe_source_msg, pa_msgobject);
+
+enum {
+PIPE_SOURCE_SUSPEND,
+PIPE_SOURCE_RESUME
+};
+
struct userdata {
pa_core *core;
pa_module *module;
@@ -71,12 +85,14 @@ struct userdata {
pa_thread_mq thread_mq;
pa_rtpoll *rtpoll;
+pipe_source_msg *msg;
+pa_usec_t timestamp;
+bool autosuspend;
+size_t pipe_size;
+
char *filename;
+int corkfd;
int fd;
-
-pa_memchunk memchunk;
-
-pa_rtpoll_item *rtpoll_item;
};
static const char* const valid_modargs[] = {
@@ -87,9 +103,41 @@ static const char* const valid_modargs[] = {
"rate",
"channels",
"channel_map",
+"autosuspend",
NULL
};
+/* Called from main context */
+static int pipe_source_process_msg(
+pa_msgobject *o,
+int code,
+void *data,
+int64_t offset,
+pa_memchunk *chunk) {
+
+struct userdata *u = (struct userdata *) data;
+
+pa_assert(u);
+
+switch (code) {
+case PIPE_SOURCE_SUSPEND:
+pa_log_debug("Suspending source %s because no writers left",
u->source->name);
+pa_source_suspend(u->source, true, PA_SUSPEND_APPLICATION);
+break;
+
+case PIPE_SOURCE_RESUME:
+pa_log_debug("Resuming source %s", u->source->name);
+pa_source_suspend(u->source, false, PA_SUSPEND_APPLICATION);
+break;
+
+default:
+pa_assert_not_reached();
+}
+
+return 0;
+}
+
+/* Called from thread context */
static int source_process_msg(
pa_msgobject *o,
int code,
@@ -101,17 +149,19 @@ static int source_process_msg(
switch (code) {
-case PA_SOURCE_MESSAGE_GET_LATENCY: {
-size_t n = 0;
+case PA_SOURCE_MESSAGE_SET_STATE:
-#ifdef FIONREAD
-int l;
+if (u->source->thread_info.state == PA_SOURCE_SUSPENDED ||
u->source->thread_info.state == PA_SOURCE_INIT) {
+if (PA_SOURCE_IS_OPENED(PA_PTR_TO_UINT(data)))
+u->timestamp = pa_rtclock_now();
+}
-if (ioctl(u->fd, FIONREAD, &l) >= 0 && l > 0)
-n = (size_t) l;
-#endif
+break;
+
+case PA_SOURCE_MESSAGE_GET_LATENCY: {
+
+*((int64_t*) data) = PA_CLIP_SUB((int64_t)pa_rtclock_now(),
(int64_t)u->timestamp);
-*((int64_t*) data) = pa_bytes_to_usec(n, &u->source->sample_spec);
return 0;
}
}
@@ -121,7 +171,11 @@ static int source_process_msg(
static void thread_func(void *userdata) {
struct userdata *u = userdata;
+pa_rtpoll_item *rtpoll_item;
+struct pollfd *pollfd;
+pa_memchunk chunk;
int read_type = 0;
+size_t fs;
pa_assert(u);
@@ -129,68 +183,140 @@ static void thread_func(void *userdata) {
pa_thread_mq_install(&u->thread_mq);
+fs = pa_frame_size(&u->source->sample_spec);
+
+pa_memchunk_reset(&chunk);
+chunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
+
+rtpoll_item = NULL;
+pollfd = NULL;
+
+u->timestamp = pa_rtclock_now();
+
+/* Close our writer here to suspend source if no writers left */
+pa_assert_se(pa_close(u->corkfd) == 0);
+u->corkfd = -1;
+
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
