Re: [pulseaudio-discuss] [PATCH] bluez5-device: Fix memory leak in sco_process_render()
Hello,
On 04/10/2018 01:38 PM, Georg Chini wrote:
> On 10.04.2018 10:21, Raman Shishniou wrote:
>> Hello,
>>
>> On 04/09/2018 07:15 PM, Georg Chini wrote:
>>> sco_process_render does not unref the memblock when it encounters an error.
>>>
>>> This patch fixes the issue. It also changes the return value to 1 in the
>>> case
>>> of EAGAIN. Because the data was already rendered and cannot be re-sent, we
>>> have to discard the block.
>>> ---
>>> src/modules/bluetooth/module-bluez5-device.c | 12 +---
>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/modules/bluetooth/module-bluez5-device.c
>>> b/src/modules/bluetooth/module-bluez5-device.c
>>> index 95d288ef..b81c233c 100644
>>> --- a/src/modules/bluetooth/module-bluez5-device.c
>>> +++ b/src/modules/bluetooth/module-bluez5-device.c
>>> @@ -282,9 +282,13 @@ static int sco_process_render(struct userdata *u) {
>>> if (errno == EINTR)
>>> /* Retry right away if we got interrupted */
>>> continue;
>>> -else if (errno == EAGAIN)
>>> -/* Hmm, apparently the socket was not writable, give up for
>>> now */
>>> -return 0;
>>> +
>>> +pa_memblock_unref(memchunk.memblock);
>>> +
>>> +if (errno == EAGAIN)
>>> +/* Hmm, apparently the socket was not writable, give up for
>>> now.
>>> + * Because the data was already rendered, let's discard the
>>> block. */
>>> +return 1;
>>>
>> 1. errno value can be changed during pa_memblock_unref()
>
> I don't think so. The only possible system calls used during unref should be
> calls to free() and because we always use pa_xfree(), the errno value will
> be preserved.
>
I seen callback processing during memblock_free()
>> 2. I think the same changes are required for a2dp_process_render() too.
>
> No, a2dp_process_render() works slightly different. It keeps the memchunk
> in userdata and tries to re-send the same block again.
>
>>
>>
>>> pa_log_error("Failed to write data to SCO socket: %s",
>>> pa_cstrerror(errno));
>>> return -1;
>>> @@ -296,6 +300,8 @@ static int sco_process_render(struct userdata *u) {
>>> pa_log_error("Wrote memory block to socket only partially! %llu
>>> written, wanted to write %llu.",
>>> (unsigned long long) l,
>>> (unsigned long long) memchunk.length);
>>> +
>>> +pa_memblock_unref(memchunk.memblock);
>>> return -1;
>>> }
>>>
>> --
>> Raman
>
>
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] bluez5-device: Fix memory leak in sco_process_render()
Hello,
On 04/09/2018 07:15 PM, Georg Chini wrote:
> sco_process_render does not unref the memblock when it encounters an error.
>
> This patch fixes the issue. It also changes the return value to 1 in the case
> of EAGAIN. Because the data was already rendered and cannot be re-sent, we
> have to discard the block.
> ---
> src/modules/bluetooth/module-bluez5-device.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/modules/bluetooth/module-bluez5-device.c
> b/src/modules/bluetooth/module-bluez5-device.c
> index 95d288ef..b81c233c 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -282,9 +282,13 @@ static int sco_process_render(struct userdata *u) {
> if (errno == EINTR)
> /* Retry right away if we got interrupted */
> continue;
> -else if (errno == EAGAIN)
> -/* Hmm, apparently the socket was not writable, give up for now
> */
> -return 0;
> +
> +pa_memblock_unref(memchunk.memblock);
> +
> +if (errno == EAGAIN)
> +/* Hmm, apparently the socket was not writable, give up for now.
> + * Because the data was already rendered, let's discard the
> block. */
> +return 1;
>
1. errno value can be changed during pa_memblock_unref()
2. I think the same changes are required for a2dp_process_render() too.
> pa_log_error("Failed to write data to SCO socket: %s",
> pa_cstrerror(errno));
> return -1;
> @@ -296,6 +300,8 @@ static int sco_process_render(struct userdata *u) {
> pa_log_error("Wrote memory block to socket only partially! %llu
> written, wanted to write %llu.",
> (unsigned long long) l,
> (unsigned long long) memchunk.length);
> +
> +pa_memblock_unref(memchunk.memblock);
> return -1;
> }
>
>
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] pipe-sink: auto-drain the pipe on sink resume
On 02/26/2018 11:06 PM, Samo Pogačnik wrote: > Dne 26.02.2018 (pon) ob 11:49 +0200 je Tanu Kaskinen napisal(a): >> On Sat, 2018-02-24 at 17:41 +0100, Samo Pogačnik wrote: >>> >>> Added option auto_drain_pipe_on_resume to enable draining any >>> remaining >>> data from the pipe upon every pipe-sink resume out of suspend. >>> --- >>> src/modules/module-pipe-sink.c | 37 >>> - >>> 1 file changed, 36 insertions(+), 1 deletion(-) >> I didn't review the patch yet, I just wanted to point out that the >> commit message doesn't explain what problem the patch solves. What's >> your use case? >> > When a pipe reader fails, the pipe sink fills up the pipe and starts > dropping instead of writing new data. Old data remains in the pipe to > be consumed by the eventually recovered or replaced reader. By each new > drop a gap between the pipe content and new data to be written grows. > If the sink suspends while dropping, resuming from suspend is going to > clear the pipe and start writing new data into an empty pipe, thus > removing the gap (old, potentially irrelevant data). > > I had also prepared a supplementary change (not yet posted), which > enables the pipe to auto-leak at the consumers end, whenever a pipe is > full. When a pipe is full, the size of the unwritten data is first read > to make room for the repeated write. This way dropping would be > replaced by leaking and the pipe always contains latest data > independent of when a real consumer reader get started, stopped or > replaced, ... However, i am not sure how to preserve content format, > if necessary. > > I thought at some point, that this second change might be sufficient, > however it makes sense (in terms of pipe content relevance and data > processing time) to drain the whole pipe upon each resume from suspend. > If the pipe reader fails (crashed?), how can the sink be suspended? Why the reader can't just drain the pipe before resume? > regards, Samo > ___ > pulseaudio-discuss mailing list > pulseaudio-discuss@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss > ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v9] pipe-source: [DRAFT] implement autosuspend behavior
On 02/23/2018 05:06 PM, Georg Chini wrote:
> On 23.02.2018 13:54, Raman Shishniou wrote:
>> On 02/23/2018 01:26 PM, Georg Chini wrote:
>>> On 23.02.2018 11:03, Raman Shishniou wrote:
>>>> On 02/23/2018 11:38 AM, Georg Chini wrote:
>>>>
>>>>> But now I have another issue:
>>>>> You are polling the pipe and running the loop even if the source is user
>>>>> suspended.
>>>>> This seems like a waste of CPU (even more than accepting some POLLIN spam
>>>>> during wakeup transition). I know you do it to discard data that is
>>>>> written during
>>>>> suspend, but I wonder if there is no better way to discard that data
>>>>> without running
>>>>> the loop permanently.
>>>>> I am thinking of draining the pipe in the SET_STATE handler. If you are
>>>>> setting
>>>>> events = 0 and open the corkfd on user suspend, nothing except messages
>>>>> should wake us up. Now, when the state changes to running, you can drain
>>>>> the
>>>>> pipe in the SET_STATE handler. The thread loop will just run through on
>>>>> the first
>>>>> iteration after user suspend, because revents = 0 and chunk.length = 0.
>>>>> Now,
>>>>> because the source is open again, you can set events = POLLIN.
>>>>> After that, you are back to normal.
>>>>> You can safely assume writer_connected=false during user suspend (you do
>>>>> not notice when the writer disconnects if you set events = 0). If the
>>>>> writer
>>>>> is still connected after suspend, writer_connected will get set when you
>>>>> read
>>>>> the first data. It will cause an unnecessary unsuspend message, but this
>>>>> will
>>>>> have no effect because neither the suspend cause nor the state change.
>>>>>
>>>>> I would also suggest to use a flag like set_pollin in the comparison, set
>>>>> and reset
>>>>> the flag in the appropriate places and explain why in a comment. This is
>>>>> one of
>>>>> the situations, where a little bit more code could make the concept
>>>>> clearer. I don't
>>>>> mind keeping it as is however, if you think it's not worth the effort.
>>>>>
>>>> We will face two main problems if we do something like that:
>>>>
>>>> First problem - we don't know how writer will react to full pipe.
>>>> If it open pipe in non-blocking mode, it will get EAGAIN on every
>>>> write() while pipe stays full. If it open pipe in blocking mode,
>>>> it will just stuck at write() until user unsuspend the source.
>>>> I think both behaviors are bad for writer - it should contain a
>>>> special code to deal with it.
>>> I guess that should be the problem of the writer. If it is intended
>>> to write to a pipe, it must be able to deal with the situation that
>>> the pipe is full.
>>>
>> Ok, but it can just ignore EAGAIN/EWOULDBLOCK while writing to pipe
>> and we'll can get broken stream when user unsuspend source.
>> Writer should do the same handling - if it got EAGAIN/EWOULDBLOCK,
>> it should drop the buffer keeping the last frame, move it to the head
>> of next chunk and try to write next chunk. Each writer should do
>> something like this in order to be ready for pipe suspend.
>>
>> Actually this it very cheap to do poll, read data, copy some bytes
>> inside a buffer and poll again. This is about several megabits.
>>
>> For example, I do this on my laptop:
>> $ mkfifo testfifo
>> $ dd if=testfifo of=/dev/null bs=512
>>
>> And on second terminal:
>> $ timeout 5 cat /dev/zero > testfifo
>>
>> Results:
>> 6991136+0 records in
>> 6991136+0 records out
>> 3579461632 bytes (3.6 GB) copied, 5.0019 s, 716 MB/s
>>
>> I.e. 6991136 * 8 * 512 / 5 / 10 ~ 5.72 Gbps
>> And 6991136 / 5 ~ 1398227 read() -> write() pairs per second
>
> This is on your machine. I just talked to somebody who is
> using PA on a 300 MHz single core MIPS system. And there
> I guess the load would be significant.
>>
I did some tests on Allwinner H3 @ 312MHz:
$ cat poll-read-test.c
#include
#include
#include
#include
#include
#include
#include
int main(void) {
struct pollfd pollfd;
char buffer[4096];
int cnt = 0;
ssize_t ret;
pollfd.fd = open("/tmp/fifo", O_RDONLY)
Re: [pulseaudio-discuss] [PATCH v9] pipe-source: [DRAFT] implement autosuspend behavior
On 02/23/2018 05:06 PM, Georg Chini wrote: > On 23.02.2018 13:54, Raman Shishniou wrote: >> On 02/23/2018 01:26 PM, Georg Chini wrote: >>> On 23.02.2018 11:03, Raman Shishniou wrote: >>>> On 02/23/2018 11:38 AM, Georg Chini wrote: >>>> >>>>> But now I have another issue: >>>>> You are polling the pipe and running the loop even if the source is user >>>>> suspended. >>>>> This seems like a waste of CPU (even more than accepting some POLLIN spam >>>>> during wakeup transition). I know you do it to discard data that is >>>>> written during >>>>> suspend, but I wonder if there is no better way to discard that data >>>>> without running >>>>> the loop permanently. >>>>> I am thinking of draining the pipe in the SET_STATE handler. If you are >>>>> setting >>>>> events = 0 and open the corkfd on user suspend, nothing except messages >>>>> should wake us up. Now, when the state changes to running, you can drain >>>>> the >>>>> pipe in the SET_STATE handler. The thread loop will just run through on >>>>> the first >>>>> iteration after user suspend, because revents = 0 and chunk.length = 0. >>>>> Now, >>>>> because the source is open again, you can set events = POLLIN. >>>>> After that, you are back to normal. >>>>> You can safely assume writer_connected=false during user suspend (you do >>>>> not notice when the writer disconnects if you set events = 0). If the >>>>> writer >>>>> is still connected after suspend, writer_connected will get set when you >>>>> read >>>>> the first data. It will cause an unnecessary unsuspend message, but this >>>>> will >>>>> have no effect because neither the suspend cause nor the state change. >>>>> >>>>> I would also suggest to use a flag like set_pollin in the comparison, set >>>>> and reset >>>>> the flag in the appropriate places and explain why in a comment. This is >>>>> one of >>>>> the situations, where a little bit more code could make the concept >>>>> clearer. I don't >>>>> mind keeping it as is however, if you think it's not worth the effort. >>>>> >>>> We will face two main problems if we do something like that: >>>> >>>> First problem - we don't know how writer will react to full pipe. >>>> If it open pipe in non-blocking mode, it will get EAGAIN on every >>>> write() while pipe stays full. If it open pipe in blocking mode, >>>> it will just stuck at write() until user unsuspend the source. >>>> I think both behaviors are bad for writer - it should contain a >>>> special code to deal with it. >>> I guess that should be the problem of the writer. If it is intended >>> to write to a pipe, it must be able to deal with the situation that >>> the pipe is full. >>> >> Ok, but it can just ignore EAGAIN/EWOULDBLOCK while writing to pipe >> and we'll can get broken stream when user unsuspend source. >> Writer should do the same handling - if it got EAGAIN/EWOULDBLOCK, >> it should drop the buffer keeping the last frame, move it to the head >> of next chunk and try to write next chunk. Each writer should do >> something like this in order to be ready for pipe suspend. >> >> Actually this it very cheap to do poll, read data, copy some bytes >> inside a buffer and poll again. This is about several megabits. >> >> For example, I do this on my laptop: >> $ mkfifo testfifo >> $ dd if=testfifo of=/dev/null bs=512 >> >> And on second terminal: >> $ timeout 5 cat /dev/zero > testfifo >> >> Results: >> 6991136+0 records in >> 6991136+0 records out >> 3579461632 bytes (3.6 GB) copied, 5.0019 s, 716 MB/s >> >> I.e. 6991136 * 8 * 512 / 5 / 10 ~ 5.72 Gbps >> And 6991136 / 5 ~ 1398227 read() -> write() pairs per second > > This is on your machine. I just talked to somebody who is > using PA on a 300 MHz single core MIPS system. And there > I guess the load would be significant. I'll check poll - read loop on my Orange PI H3 and Amlogic S805 slowed down to 300 MHz tonight :) >> >>>> The second problem is hidden now because I temporary dropped >>>> a part of code that keep frame boundaries. If we drain the pipe >>>> as soon as
Re: [pulseaudio-discuss] [PATCH v9] pipe-source: [DRAFT] implement autosuspend behavior
On 02/23/2018 01:26 PM, Georg Chini wrote: > On 23.02.2018 11:03, Raman Shishniou wrote: >> On 02/23/2018 11:38 AM, Georg Chini wrote: >> >>> But now I have another issue: >>> You are polling the pipe and running the loop even if the source is user >>> suspended. >>> This seems like a waste of CPU (even more than accepting some POLLIN spam >>> during wakeup transition). I know you do it to discard data that is written >>> during >>> suspend, but I wonder if there is no better way to discard that data >>> without running >>> the loop permanently. >>> I am thinking of draining the pipe in the SET_STATE handler. If you are >>> setting >>> events = 0 and open the corkfd on user suspend, nothing except messages >>> should wake us up. Now, when the state changes to running, you can drain the >>> pipe in the SET_STATE handler. The thread loop will just run through on the >>> first >>> iteration after user suspend, because revents = 0 and chunk.length = 0. Now, >>> because the source is open again, you can set events = POLLIN. >>> After that, you are back to normal. >>> You can safely assume writer_connected=false during user suspend (you do >>> not notice when the writer disconnects if you set events = 0). If the writer >>> is still connected after suspend, writer_connected will get set when you >>> read >>> the first data. It will cause an unnecessary unsuspend message, but this >>> will >>> have no effect because neither the suspend cause nor the state change. >>> >>> I would also suggest to use a flag like set_pollin in the comparison, set >>> and reset >>> the flag in the appropriate places and explain why in a comment. This is >>> one of >>> the situations, where a little bit more code could make the concept >>> clearer. I don't >>> mind keeping it as is however, if you think it's not worth the effort. >>> >> We will face two main problems if we do something like that: >> >> First problem - we don't know how writer will react to full pipe. >> If it open pipe in non-blocking mode, it will get EAGAIN on every >> write() while pipe stays full. If it open pipe in blocking mode, >> it will just stuck at write() until user unsuspend the source. >> I think both behaviors are bad for writer - it should contain a >> special code to deal with it. > > I guess that should be the problem of the writer. If it is intended > to write to a pipe, it must be able to deal with the situation that > the pipe is full. > Ok, but it can just ignore EAGAIN/EWOULDBLOCK while writing to pipe and we'll can get broken stream when user unsuspend source. Writer should do the same handling - if it got EAGAIN/EWOULDBLOCK, it should drop the buffer keeping the last frame, move it to the head of next chunk and try to write next chunk. Each writer should do something like this in order to be ready for pipe suspend. Actually this it very cheap to do poll, read data, copy some bytes inside a buffer and poll again. This is about several megabits. For example, I do this on my laptop: $ mkfifo testfifo $ dd if=testfifo of=/dev/null bs=512 And on second terminal: $ timeout 5 cat /dev/zero > testfifo Results: 6991136+0 records in 6991136+0 records out 3579461632 bytes (3.6 GB) copied, 5.0019 s, 716 MB/s I.e. 6991136 * 8 * 512 / 5 / 10 ~ 5.72 Gbps And 6991136 / 5 ~ 1398227 read() -> write() pairs per second >> >> The second problem is hidden now because I temporary dropped >> a part of code that keep frame boundaries. If we drain the pipe >> as soon as user resume the source - we'll loose frame boundaries. >> Audio stream will be broken for any case except s8/u8 mono. >> Actually we have to read every time while suspended by user and drop >> whole chunk except for not completely read last frame, move it to >> the head of memchunk and do next read() at position where this frame ends. > > We could work around this I think. You just need to have the last > read fragment available in the SET_STATE handler. Then you do > not loose frame boundaries, because you continue to read where > you have stopped. > Yep, but after resume it will be part of another frame. And only if writer does right things while we a suspended. >> >> BTW, currently pipe-source PA just crashed if I try to write s24le to pipe: >> >> I decided to do not open a bug because almost whole pipe-source should >> be rewritten, and this is what I'm doing now. >> > Yes, makes sense. If your patch fixes a crash bug, it has a good > chance to get into 12.0 (which it would not have otherwise). It depends on whether Tanu's patches come to the master or not. -- Raman ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v9] pipe-source: [DRAFT] implement autosuspend behavior
On 02/23/2018 11:38 AM, Georg Chini wrote:
> On 22.02.2018 22:01, Raman Shishniou wrote:
>> On 02/22/2018 10:18 PM, Georg Chini wrote:
>>>> -/* Hmm, nothing to do. Let's sleep */
>>>> -pollfd->events = (short) (u->source->thread_info.state ==
>>>> PA_SOURCE_RUNNING ? POLLIN : 0);
>>>> +/* Post data to source, discard data or wait for state transition
>>>> to be complete */
>>>> +if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
>>>> +
>>>> +if (u->writer_connected && u->corkfd >= 0) {
>>>> +pa_assert_se(pa_close(u->corkfd) == 0);
>>>> +u->corkfd = -1;
>>>> +}
>>>> +
>>>> +if (u->memchunk.length > 0) {
>>>> +pa_source_post(u->source, &u->memchunk);
>>>> +pa_memblock_unref(u->memchunk.memblock);
>>>> +u->memchunk.memblock = pa_memblock_new(u->core->mempool,
>>>> u->pipe_size);
>>>> +u->memchunk.length = 0;
>>>> +}
>>>> +
>>>> +} else if (u->suspended_by_user)
>>> You have to open the corkfd here as well to avoid POLLHUP spam if the writer
>>> disconnects during user suspend.
>>>
>> During user suspend we are doing normal polling. If the writer will be
>> disconnected
>> we'll see it by reading 0 bytes and corkfd will be opened.
>
> See below, I would like to avoid continuous polling during user suspend.
>
>>
>>>> +u->memchunk.length = 0;
>>>> +
>>>> +
>>>> +pollfd->events = u->memchunk.length ? 0 : POLLIN;
>>> How do you wake up from autosuspend? When the writer disconnects,
>>> memchunk.length will be 0 and so you do no longer listen for POLLIN.
>>> I guess you need to know the current suspend cause here:
>> > user suspended -> events = 0
>>> (only auto suspended + no writer connected) or running -> events = POLLIN
>>> only auto suspended + writer connected (=wake up transition) -> events = 0
>>>
>> Yes. This is what I do, keep data in memchunk and wait while source will be
>> resumed.
>>
> Yeah, looks I am too dump to understand a simple comparison ... sorry. (Shame
> on
> me, I know I have been making a lot of silly mistakes during my reviews and
> your
> knowledge of PA is better than mine. I hope I did not annoy you too much.)
>
I'm sure you are not dump. I know only small part of PA for now and
only thanks to your reviews :)
> But now I have another issue:
> You are polling the pipe and running the loop even if the source is user
> suspended.
> This seems like a waste of CPU (even more than accepting some POLLIN spam
> during wakeup transition). I know you do it to discard data that is written
> during
> suspend, but I wonder if there is no better way to discard that data without
> running
> the loop permanently.
> I am thinking of draining the pipe in the SET_STATE handler. If you are
> setting
> events = 0 and open the corkfd on user suspend, nothing except messages
> should wake us up. Now, when the state changes to running, you can drain the
> pipe in the SET_STATE handler. The thread loop will just run through on the
> first
> iteration after user suspend, because revents = 0 and chunk.length = 0. Now,
> because the source is open again, you can set events = POLLIN.
> After that, you are back to normal.
> You can safely assume writer_connected=false during user suspend (you do
> not notice when the writer disconnects if you set events = 0). If the writer
> is still connected after suspend, writer_connected will get set when you read
> the first data. It will cause an unnecessary unsuspend message, but this will
> have no effect because neither the suspend cause nor the state change.
>
> I would also suggest to use a flag like set_pollin in the comparison, set and
> reset
> the flag in the appropriate places and explain why in a comment. This is one
> of
> the situations, where a little bit more code could make the concept clearer.
> I don't
> mind keeping it as is however, if you think it's not worth the effort.
>
We will face two main problems if we do something like that:
First problem - we don't know how writer will react to full pipe.
If it open pipe in non-blocking mode, it will get EAGAIN on every
write() while pipe stays full. If it open pipe in blocking mode,
it will just st
Re: [pulseaudio-discuss] [PATCH v9] pipe-source: [DRAFT] implement autosuspend behavior
On 02/22/2018 10:18 PM, Georg Chini wrote:
>> -/* Hmm, nothing to do. Let's sleep */
>> -pollfd->events = (short) (u->source->thread_info.state ==
>> PA_SOURCE_RUNNING ? POLLIN : 0);
>> +/* Post data to source, discard data or wait for state transition
>> to be complete */
>> +if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
>> +
>> +if (u->writer_connected && u->corkfd >= 0) {
>> +pa_assert_se(pa_close(u->corkfd) == 0);
>> +u->corkfd = -1;
>> +}
>> +
>> +if (u->memchunk.length > 0) {
>> +pa_source_post(u->source, &u->memchunk);
>> +pa_memblock_unref(u->memchunk.memblock);
>> +u->memchunk.memblock = pa_memblock_new(u->core->mempool,
>> u->pipe_size);
>> +u->memchunk.length = 0;
>> +}
>> +
>> +} else if (u->suspended_by_user)
>
> You have to open the corkfd here as well to avoid POLLHUP spam if the writer
> disconnects during user suspend.
>
During user suspend we are doing normal polling. If the writer will be
disconnected
we'll see it by reading 0 bytes and corkfd will be opened.
>> +u->memchunk.length = 0;
>> +
>> +
>> +pollfd->events = u->memchunk.length ? 0 : POLLIN;
>
> How do you wake up from autosuspend? When the writer disconnects,
> memchunk.length will be 0 and so you do no longer listen for POLLIN.
> I guess you need to know the current suspend cause here:
> user suspended -> events = 0
> (only auto suspended + no writer connected) or running -> events = POLLIN
> only auto suspended + writer connected (=wake up transition) -> events = 0
>
Yes. This is what I do, keep data in memchunk and wait while source will be
resumed.
- Source autosuspended, corkfd opened, rtpoll wait for POLLIN
- Someone connected and send us data.
- pa_rtpoll_run() returned and pollfd->revents = POLLIN
First iteration:
- pollfd->revents == POLLIN
- l = read()
- l > 0:
- memchunk.length = l
- writer_connected is false
- send PIPE_SOURCE_RESUME message
- set writer_connected = true,
! do not close corkfd here to avoid POLLHUP spam !
- Data cannot be posted because source still suspended, i.e. we just send
resume message to main thread and thread_info.state not changed yet.
- Data cannot be discarded - source auto suspended, not suspended by user.
- So we need to wait for source to be resumed (and keep data in memchunk):
- events = 0
- pa_rtpoll_run(): corkfd still opened, but events = 0 - we are waiting for our
resume message will be processed.
Next iteration
- resume message was processed
- pollfd->events = 0 (pellfb->events was set to 0 in previous iteration)
- Source was resumed:
- writer_connected is true and corkfd still opened:
-close corkfd
- post data to source
- allocate new buffer
- memchunk.length = 0
- We are processed pending data (memchunk.length == 0)
- set events = POLLIN
- pa_rtpoll_run(): corkfd closed, events = POLLIN - we are waiting for new data
>> if ((ret = pa_rtpoll_run(u->rtpoll)) < 0)
>> goto fail;
>> @@ -188,8 +281,13 @@ static void thread_func(void *userdata) {
>> pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>> -if (pollfd->revents & ~POLLIN) {
>> -pa_log("FIFO shutdown.");
>> +if (pollfd->revents & ~(POLLIN|POLLHUP)) {
>> +pa_log("FD error: %s%s%s%s",
>> + pollfd->revents & POLLOUT ? "POLLOUT " : "",
>> + pollfd->revents & POLLERR ? "POLLERR " : "",
>> + pollfd->revents & POLLPRI ? "POLLPRI " : "",
>> + pollfd->revents & POLLNVAL ? "POLLNVAL " : "");
>> +
>> goto fail;
>> }
>> }
>> @@ -238,13 +336,26 @@ int pa__init(pa_module *m) {
>> goto fail;
>> }
>> +if (!(u->msg = pa_msgobject_new(pipe_source_msg)))
>> +goto fail;
>> +
>> +u->msg->parent.process_msg = pipe_source_process_msg;
>> +
>> u->filename = pa_runtime_path(pa_modargs_get_value(ma, "file",
>> DEFAULT_FILE_NAME));
>> if (mkfifo(u->filename, 0666) < 0) {
>> pa_log("mkfifo('%s'): %s", u->filename, pa_cstrerror(errno));
>> goto fail;
>> }
>> -if ((u->fd = pa_open_cloexec(u->filename, O_RDWR, 0)) < 0) {
>> +
>> +if ((u->corkfd = pa_open_cloexec(u->filename, O_RDWR, 0)) < 0) {
>> +pa_log("open('%s'): %s", u->filename, pa_cstrerror(errno));
>> +goto fail;
>> +}
>> +
>> +u->writer_connected = true;
>> +
>> +if ((u->fd = pa_open_cloexec(u->filename, O_RDONLY, 0)) < 0) {
>> pa_log("open('%s'): %s", u->filename, pa_cstrerror(errno));
>> goto fail;
>> }
>> @@ -289,13 +400,18 @@ int pa__init(pa_module *m) {
>> pa_source_set_asyncmsgq(u->source, u->thread_mq.inq);
>> pa_source_set_rtpoll(u->source, u->rtpoll);
>> -p
Re: [pulseaudio-discuss] [PATCH v9] pipe-source: [DRAFT] implement autosuspend behavior
On 02/22/2018 07:15 PM, Raman Shyshniou wrote: > Currently the pipe-source will remain running even if no > writer is connected and therefore no data is produced. > This patch prevets this by auto-suspending source > when all writers are disconnected. This a draft. I applied Tanu's patch to next branch [PATCH 6/8] pass pa_suspend_cause_t to SINK/SOURCE_SET_STATE handlers to make it possible to track a suspend cause. So tracking part should be rewritten when Tanu push his new patch with set_state_in_io_thread() callbacks. Also I temporary dropped autosuspend option and frame size keeping code to make my code simple. I'll add them later. -- 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/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 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_inf
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
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
>
Re: [pulseaudio-discuss] [PATCH 6/8] pass pa_suspend_cause_t to SINK/SOURCE_SET_STATE handlers
On 02/19/2018 05:48 PM, Tanu Kaskinen wrote:
> The suspend cause isn't yet used by any of the handlers. The alsa sink
> and source will use it to sync the mixer when the SESSION suspend cause
> is removed. Currently the syncing is done in pa_sink/source_suspend(),
> and I want to change that, because pa_sink/source_suspend() shouldn't
> have any alsa specific code.
> ---
> @@ -2845,12 +2848,12 @@ int pa_sink_process_msg(pa_msgobject *o, int code,
> void *userdata, int64_t offse
> return 0;
>
> case PA_SINK_MESSAGE_SET_STATE: {
> -
> +pa_sink_state_t new_state = ((pa_sink_message_set_state *)
> userdata)->state;
I think we can save suspend_cause to thread_info too:
+pa_suspend_cause_t suspend_cause = ((pa_sink_message_set_state *)
userdata)->suspend_cause;
> bool suspend_change =
> -(s->thread_info.state == PA_SINK_SUSPENDED &&
> PA_SINK_IS_OPENED(PA_PTR_TO_UINT(userdata))) ||
> -(PA_SINK_IS_OPENED(s->thread_info.state) &&
> PA_PTR_TO_UINT(userdata) == PA_SINK_SUSPENDED);
> +(s->thread_info.state == PA_SINK_SUSPENDED &&
> PA_SINK_IS_OPENED(new_state)) ||
> +(PA_SINK_IS_OPENED(s->thread_info.state) && new_state ==
> PA_SINK_SUSPENDED);
>
> -s->thread_info.state = PA_PTR_TO_UINT(userdata);
> +s->thread_info.state = new_state;
+s->thread_info.suspend_cause = suspend_cause;
>
> if (s->thread_info.state == PA_SINK_SUSPENDED) {
> s->thread_info.rewind_nbytes = 0;
...
> @@ -2219,12 +,12 @@ int pa_source_process_msg(pa_msgobject *object, int
> code, void *userdata, int64_
> return 0;
>
> case PA_SOURCE_MESSAGE_SET_STATE: {
> -
> +pa_source_state_t new_state = ((pa_source_message_set_state *)
> userdata)->state;
+pa_source_state_t suspend_cause = ((pa_source_message_set_state *)
userdata)->suspend_cause;
> bool suspend_change =
> -(s->thread_info.state == PA_SOURCE_SUSPENDED &&
> PA_SOURCE_IS_OPENED(PA_PTR_TO_UINT(userdata))) ||
> -(PA_SOURCE_IS_OPENED(s->thread_info.state) &&
> PA_PTR_TO_UINT(userdata) == PA_SOURCE_SUSPENDED);
> +(s->thread_info.state == PA_SOURCE_SUSPENDED &&
> PA_SOURCE_IS_OPENED(new_state)) ||
> +(PA_SOURCE_IS_OPENED(s->thread_info.state) && new_state ==
> PA_SOURCE_SUSPENDED);
>
> -s->thread_info.state = PA_PTR_TO_UINT(userdata);
> +s->thread_info.state = new_state;
+s->thread_info.suspend_cause = suspend_cause;
>
> if (suspend_change) {
> pa_source_output *o;
And add suspend_cause to sink's and source's thread_info and init:
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index 7f5c37f..3904158 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -330,6 +330,7 @@ pa_sink* pa_sink_new(
s->thread_info.soft_volume = s->soft_volume;
s->thread_info.soft_muted = s->muted;
s->thread_info.state = s->state;
+s->thread_info.suspend_cause = s->suspend_cause;
s->thread_info.rewind_nbytes = 0;
s->thread_info.rewind_requested = false;
s->thread_info.max_rewind = 0;
diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index 3fb2301..e897d69 100644
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -252,6 +252,7 @@ struct pa_sink {
* thread can work without access locking */
struct {
pa_sink_state_t state;
+pa_suspend_cause_t suspend_cause;
pa_hashmap *inputs;
pa_rtpoll *rtpoll;
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index 2a6b1f1..4eaa1a9 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -318,6 +318,7 @@ pa_source* pa_source_new(
s->thread_info.soft_volume = s->soft_volume;
s->thread_info.soft_muted = s->muted;
s->thread_info.state = s->state;
+s->thread_info.suspend_cause = s->suspend_cause;
s->thread_info.max_rewind = 0;
s->thread_info.requested_latency_valid = false;
s->thread_info.requested_latency = 0;
diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
index 75ce241..cfe4306 100644
--- a/src/pulsecore/source.h
+++ b/src/pulsecore/source.h
@@ -210,6 +210,7 @@ struct pa_source {
* thread can work without access locking */
struct {
pa_source_state_t state;
+pa_suspend_cause_t suspend_cause;
pa_hashmap *outputs;
pa_rtpoll *rtpoll;
--
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/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 i
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_susp
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 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 && po
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 ...
&g
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 v7] pipe-source: implement autosuspend option
On 02/20/2018 07:14 PM, Georg Chini wrote:
> On 20.02.2018 17:08, Raman Shishniou wrote:
>> On 02/20/2018 06:44 PM, Georg Chini wrote:
>>> On 20.02.2018 16:28, Raman Shishniou wrote:
>>>> On 02/20/2018 06:22 PM, Georg Chini wrote:
>>>>> On 20.02.2018 16:13, Georg Chini wrote:
>>>>>> On 20.02.2018 15:47, Raman Shishniou wrote:
>>>>>>> On 02/20/2018 04:55 PM, Georg Chini wrote:
>>>>>>>> On 20.02.2018 14:30, Raman Shishniou wrote:
>>>>>>>>> On 02/20/2018 03:11 PM, Georg Chini wrote:
>>>>>>>>>> On 19.02.2018 16:01, Raman Shyshniou wrote:
>>>>>>>>>>> Currently the pipe-source will remain running even if no
>>>>>>>>>>> writer is connected and therefore no data is produced.
>>>>>>>>>>> This patch adds the autosuspend= option to prevent this.
>>>>>>>>>>> Source will stay suspended if no writer is connected.
>>>>>>>>>>> This option is enabled by default.
>>>>>>>>>>> ---
>>>>>>>>>>> src/modules/module-pipe-source.c | 279
>>>>>>>>>>> ++-
>>>>>>>>>>> 1 file changed, 215 insertions(+), 64 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> for (;;) {
>>>>>>>>>>> int ret;
>>>>>>>>>>> -struct pollfd *pollfd;
>>>>>>>>>>> -pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item,
>>>>>>>>>>> NULL);
>>>>>>>>>>> +if (chunk.length) {
>>>>>>>>>>> +/* We have a pending data, let's stop polling pipe.
>>>>>>>>>>> + * Setting up pollfd->events = 0 is not enough to stop
>>>>>>>>>>> + * POLLHUP spam if all writers are closed pipe.
>>>>>>>>>>> + * We need to stop polling pipe completely */
>>>>>>>>>>> +if (rtpoll_item) {
>>>>>>>>>>> +pa_rtpoll_item_free(rtpoll_item);
>>>>>>>>>>> +rtpoll_item = NULL;
>>>>>>>>>>> +}
>>>>>>>>>>> +} else {
>>>>>>>>>>> +/* We have no pending data, let's start polling pipe */
>>>>>>>>>>> +if (rtpoll_item == NULL) {
>>>>>>>>>>> +rtpoll_item = pa_rtpoll_item_new(u->rtpoll,
>>>>>>>>>>> PA_RTPOLL_NEVER, 1);
>>>>>>>>>>> +pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item,
>>>>>>>>>>> NULL);
>>>>>>>>>>> +pollfd->events = POLLIN;
>>>>>>>>>>> +pollfd->fd = u->fd;
>>>>>>>>>>> +}
>>>>>>>>>>> +}
>>>>>>>>>> Why do you need to do that? As in your previous patches you
>>>>>>>>>> open the pipe for writing if all writers are disconnected, which
>>>>>>>>>> should stop POLLHUP's.
>>>>>>>>> This patch tries to be platform-independent.
>>>>>>>>> Unfortunately not all platforms set POLLHUP when a writer closed pipe.
>>>>>>>> Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
>>>>>>>> you know the pipe was closed. If you have POLLIN and l = 0 on your
>>>>>>>> read, you also know the pipe was closed.
>>>>>>>> Then you open your corkfd and stop the POLLHUP/POLLIN.
>>>>>>>> So why do you have to stop polling completely? When the next
>>>>>>>> POLLIN comes, you know that data has been written and POLLHUP
>>>>>>>> can no longer happen.
>>>>>>>>
>>>>>>> If I open pipe for writing before all data was read, i.e. when I see
>>>>>>> POLLHUP - read() will never return 0. Also sen
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 v7] pipe-source: implement autosuspend option
On 02/20/2018 06:44 PM, Georg Chini wrote:
> On 20.02.2018 16:28, Raman Shishniou wrote:
>>
>> On 02/20/2018 06:22 PM, Georg Chini wrote:
>>> On 20.02.2018 16:13, Georg Chini wrote:
>>>> On 20.02.2018 15:47, Raman Shishniou wrote:
>>>>> On 02/20/2018 04:55 PM, Georg Chini wrote:
>>>>>> On 20.02.2018 14:30, Raman Shishniou wrote:
>>>>>>> On 02/20/2018 03:11 PM, Georg Chini wrote:
>>>>>>>> On 19.02.2018 16:01, Raman Shyshniou wrote:
>>>>>>>>> Currently the pipe-source will remain running even if no
>>>>>>>>> writer is connected and therefore no data is produced.
>>>>>>>>> This patch adds the autosuspend= option to prevent this.
>>>>>>>>> Source will stay suspended if no writer is connected.
>>>>>>>>> This option is enabled by default.
>>>>>>>>> ---
>>>>>>>>> src/modules/module-pipe-source.c | 279
>>>>>>>>> ++-
>>>>>>>>> 1 file changed, 215 insertions(+), 64 deletions(-)
>>>>>>>>>
>>>>>>>>> +
>>>>>>>>> for (;;) {
>>>>>>>>> int ret;
>>>>>>>>> -struct pollfd *pollfd;
>>>>>>>>> -pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item,
>>>>>>>>> NULL);
>>>>>>>>> +if (chunk.length) {
>>>>>>>>> +/* We have a pending data, let's stop polling pipe.
>>>>>>>>> + * Setting up pollfd->events = 0 is not enough to stop
>>>>>>>>> + * POLLHUP spam if all writers are closed pipe.
>>>>>>>>> + * We need to stop polling pipe completely */
>>>>>>>>> +if (rtpoll_item) {
>>>>>>>>> +pa_rtpoll_item_free(rtpoll_item);
>>>>>>>>> +rtpoll_item = NULL;
>>>>>>>>> +}
>>>>>>>>> +} else {
>>>>>>>>> +/* We have no pending data, let's start polling pipe */
>>>>>>>>> +if (rtpoll_item == NULL) {
>>>>>>>>> +rtpoll_item = pa_rtpoll_item_new(u->rtpoll,
>>>>>>>>> PA_RTPOLL_NEVER, 1);
>>>>>>>>> +pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item,
>>>>>>>>> NULL);
>>>>>>>>> +pollfd->events = POLLIN;
>>>>>>>>> +pollfd->fd = u->fd;
>>>>>>>>> +}
>>>>>>>>> +}
>>>>>>>> Why do you need to do that? As in your previous patches you
>>>>>>>> open the pipe for writing if all writers are disconnected, which
>>>>>>>> should stop POLLHUP's.
>>>>>>> This patch tries to be platform-independent.
>>>>>>> Unfortunately not all platforms set POLLHUP when a writer closed pipe.
>>>>>> Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
>>>>>> you know the pipe was closed. If you have POLLIN and l = 0 on your
>>>>>> read, you also know the pipe was closed.
>>>>>> Then you open your corkfd and stop the POLLHUP/POLLIN.
>>>>>> So why do you have to stop polling completely? When the next
>>>>>> POLLIN comes, you know that data has been written and POLLHUP
>>>>>> can no longer happen.
>>>>>>
>>>>> If I open pipe for writing before all data was read, i.e. when I see
>>>>> POLLHUP - read() will never return 0. Also send suspend message when
>>>>> I see POLLHUP is too early. I should open pipe for writing and send
>>>>> suspend message when read() return 0.
>>>>>
>>>>>
>>>>>>> Moreover, when POLLHUP was set there may be data in pipe that must be
>>>>>>> read
>>>>>>> before suspend. Linux sets POLLIN in that case. Freebsd and macos sets
>>>>>>> POLLIN|POLLHUP even if no any data in pipe, but no writers left. Some
>>>>
Re: [pulseaudio-discuss] [PATCH v7] pipe-source: implement autosuspend option
On 02/20/2018 06:28 PM, Raman Shishniou wrote:
>
>
> On 02/20/2018 06:22 PM, Georg Chini wrote:
>> On 20.02.2018 16:13, Georg Chini wrote:
>>> On 20.02.2018 15:47, Raman Shishniou wrote:
>>>> On 02/20/2018 04:55 PM, Georg Chini wrote:
>>>>> On 20.02.2018 14:30, Raman Shishniou wrote:
>>>>>> On 02/20/2018 03:11 PM, Georg Chini wrote:
>>>>>>> On 19.02.2018 16:01, Raman Shyshniou wrote:
>>>>>>>> Currently the pipe-source will remain running even if no
>>>>>>>> writer is connected and therefore no data is produced.
>>>>>>>> This patch adds the autosuspend= option to prevent this.
>>>>>>>> Source will stay suspended if no writer is connected.
>>>>>>>> This option is enabled by default.
>>>>>>>> ---
>>>>>>>> src/modules/module-pipe-source.c | 279
>>>>>>>> ++-
>>>>>>>> 1 file changed, 215 insertions(+), 64 deletions(-)
>>>>>>>>
>>>>>>>> +
>>>>>>>> for (;;) {
>>>>>>>> int ret;
>>>>>>>> -struct pollfd *pollfd;
>>>>>>>> -pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>>>>>>>> +if (chunk.length) {
>>>>>>>> +/* We have a pending data, let's stop polling pipe.
>>>>>>>> + * Setting up pollfd->events = 0 is not enough to stop
>>>>>>>> + * POLLHUP spam if all writers are closed pipe.
>>>>>>>> + * We need to stop polling pipe completely */
>>>>>>>> +if (rtpoll_item) {
>>>>>>>> +pa_rtpoll_item_free(rtpoll_item);
>>>>>>>> +rtpoll_item = NULL;
>>>>>>>> +}
>>>>>>>> +} else {
>>>>>>>> +/* We have no pending data, let's start polling pipe */
>>>>>>>> +if (rtpoll_item == NULL) {
>>>>>>>> +rtpoll_item = pa_rtpoll_item_new(u->rtpoll,
>>>>>>>> PA_RTPOLL_NEVER, 1);
>>>>>>>> +pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item, NULL);
>>>>>>>> +pollfd->events = POLLIN;
>>>>>>>> +pollfd->fd = u->fd;
>>>>>>>> +}
>>>>>>>> +}
>>>>>>> Why do you need to do that? As in your previous patches you
>>>>>>> open the pipe for writing if all writers are disconnected, which
>>>>>>> should stop POLLHUP's.
>>>>>> This patch tries to be platform-independent.
>>>>>> Unfortunately not all platforms set POLLHUP when a writer closed pipe.
>>>>> Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
>>>>> you know the pipe was closed. If you have POLLIN and l = 0 on your
>>>>> read, you also know the pipe was closed.
>>>>> Then you open your corkfd and stop the POLLHUP/POLLIN.
>>>>> So why do you have to stop polling completely? When the next
>>>>> POLLIN comes, you know that data has been written and POLLHUP
>>>>> can no longer happen.
>>>>>
>>>> If I open pipe for writing before all data was read, i.e. when I see
>>>> POLLHUP - read() will never return 0. Also send suspend message when
>>>> I see POLLHUP is too early. I should open pipe for writing and send
>>>> suspend message when read() return 0.
>>>>
>>>>
>>>>>> Moreover, when POLLHUP was set there may be data in pipe that must be
>>>>>> read
>>>>>> before suspend. Linux sets POLLIN in that case. Freebsd and macos sets
>>>>>> POLLIN|POLLHUP even if no any data in pipe, but no writers left. Some
>>>>>> platforms
>>>>>> do not set POLLHUP at all. So do read() and check it return 0 must be
>>>>>> the only
>>>>>> condition to suspend source and open pipe for writing.
>>>>> Yes, I agree, see above, but I still don't understand why you stop
>>>>> polling.
>>>>>
>>&
Re: [pulseaudio-discuss] [PATCH v7] pipe-source: implement autosuspend option
On 02/20/2018 06:22 PM, Georg Chini wrote:
> On 20.02.2018 16:13, Georg Chini wrote:
>> On 20.02.2018 15:47, Raman Shishniou wrote:
>>> On 02/20/2018 04:55 PM, Georg Chini wrote:
>>>> On 20.02.2018 14:30, Raman Shishniou wrote:
>>>>> On 02/20/2018 03:11 PM, Georg Chini wrote:
>>>>>> On 19.02.2018 16:01, Raman Shyshniou wrote:
>>>>>>> Currently the pipe-source will remain running even if no
>>>>>>> writer is connected and therefore no data is produced.
>>>>>>> This patch adds the autosuspend= option to prevent this.
>>>>>>> Source will stay suspended if no writer is connected.
>>>>>>> This option is enabled by default.
>>>>>>> ---
>>>>>>> src/modules/module-pipe-source.c | 279
>>>>>>> ++-
>>>>>>> 1 file changed, 215 insertions(+), 64 deletions(-)
>>>>>>>
>>>>>>> +
>>>>>>> for (;;) {
>>>>>>> int ret;
>>>>>>> -struct pollfd *pollfd;
>>>>>>> -pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>>>>>>> +if (chunk.length) {
>>>>>>> +/* We have a pending data, let's stop polling pipe.
>>>>>>> + * Setting up pollfd->events = 0 is not enough to stop
>>>>>>> + * POLLHUP spam if all writers are closed pipe.
>>>>>>> + * We need to stop polling pipe completely */
>>>>>>> +if (rtpoll_item) {
>>>>>>> +pa_rtpoll_item_free(rtpoll_item);
>>>>>>> +rtpoll_item = NULL;
>>>>>>> +}
>>>>>>> +} else {
>>>>>>> +/* We have no pending data, let's start polling pipe */
>>>>>>> +if (rtpoll_item == NULL) {
>>>>>>> +rtpoll_item = pa_rtpoll_item_new(u->rtpoll,
>>>>>>> PA_RTPOLL_NEVER, 1);
>>>>>>> +pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item, NULL);
>>>>>>> +pollfd->events = POLLIN;
>>>>>>> +pollfd->fd = u->fd;
>>>>>>> +}
>>>>>>> +}
>>>>>> Why do you need to do that? As in your previous patches you
>>>>>> open the pipe for writing if all writers are disconnected, which
>>>>>> should stop POLLHUP's.
>>>>> This patch tries to be platform-independent.
>>>>> Unfortunately not all platforms set POLLHUP when a writer closed pipe.
>>>> Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
>>>> you know the pipe was closed. If you have POLLIN and l = 0 on your
>>>> read, you also know the pipe was closed.
>>>> Then you open your corkfd and stop the POLLHUP/POLLIN.
>>>> So why do you have to stop polling completely? When the next
>>>> POLLIN comes, you know that data has been written and POLLHUP
>>>> can no longer happen.
>>>>
>>> If I open pipe for writing before all data was read, i.e. when I see
>>> POLLHUP - read() will never return 0. Also send suspend message when
>>> I see POLLHUP is too early. I should open pipe for writing and send
>>> suspend message when read() return 0.
>>>
>>>
>>>>> Moreover, when POLLHUP was set there may be data in pipe that must be read
>>>>> before suspend. Linux sets POLLIN in that case. Freebsd and macos sets
>>>>> POLLIN|POLLHUP even if no any data in pipe, but no writers left. Some
>>>>> platforms
>>>>> do not set POLLHUP at all. So do read() and check it return 0 must be the
>>>>> only
>>>>> condition to suspend source and open pipe for writing.
>>>> Yes, I agree, see above, but I still don't understand why you stop polling.
>>>>
>>>>> Also it cover cases when source suspended by user - we read data from
>>>>> pipe,
>>>>> send resume, but source stays suspended. We must stop polling pipe, but
>>>>> pollfd->events = 0 is not enough. If the writer make a open-write-close
>>>>> while source stays suspended we will get endless POLLHUPs
>
Re: [pulseaudio-discuss] [PATCH v7] pipe-source: implement autosuspend option
On 02/20/2018 04:55 PM, Georg Chini wrote:
> On 20.02.2018 14:30, Raman Shishniou wrote:
>> On 02/20/2018 03:11 PM, Georg Chini wrote:
>>> On 19.02.2018 16:01, Raman Shyshniou wrote:
>>>> Currently the pipe-source will remain running even if no
>>>> writer is connected and therefore no data is produced.
>>>> This patch adds the autosuspend= option to prevent this.
>>>> Source will stay suspended if no writer is connected.
>>>> This option is enabled by default.
>>>> ---
>>>>src/modules/module-pipe-source.c | 279
>>>> ++-
>>>>1 file changed, 215 insertions(+), 64 deletions(-)
>>>>
>>>> +
>>>>for (;;) {
>>>>int ret;
>>>> -struct pollfd *pollfd;
>>>>-pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>>>> +if (chunk.length) {
>>>> +/* We have a pending data, let's stop polling pipe.
>>>> + * Setting up pollfd->events = 0 is not enough to stop
>>>> + * POLLHUP spam if all writers are closed pipe.
>>>> + * We need to stop polling pipe completely */
>>>> +if (rtpoll_item) {
>>>> +pa_rtpoll_item_free(rtpoll_item);
>>>> +rtpoll_item = NULL;
>>>> +}
>>>> +} else {
>>>> +/* We have no pending data, let's start polling pipe */
>>>> +if (rtpoll_item == NULL) {
>>>> +rtpoll_item = pa_rtpoll_item_new(u->rtpoll,
>>>> PA_RTPOLL_NEVER, 1);
>>>> +pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item, NULL);
>>>> +pollfd->events = POLLIN;
>>>> +pollfd->fd = u->fd;
>>>> +}
>>>> +}
>>> Why do you need to do that? As in your previous patches you
>>> open the pipe for writing if all writers are disconnected, which
>>> should stop POLLHUP's.
>> This patch tries to be platform-independent.
>> Unfortunately not all platforms set POLLHUP when a writer closed pipe.
>
> Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
> you know the pipe was closed. If you have POLLIN and l = 0 on your
> read, you also know the pipe was closed.
> Then you open your corkfd and stop the POLLHUP/POLLIN.
> So why do you have to stop polling completely? When the next
> POLLIN comes, you know that data has been written and POLLHUP
> can no longer happen.
>
If I open pipe for writing before all data was read, i.e. when I see
POLLHUP - read() will never return 0. Also send suspend message when
I see POLLHUP is too early. I should open pipe for writing and send
suspend message when read() return 0.
>
>> Moreover, when POLLHUP was set there may be data in pipe that must be read
>> before suspend. Linux sets POLLIN in that case. Freebsd and macos sets
>> POLLIN|POLLHUP even if no any data in pipe, but no writers left. Some
>> platforms
>> do not set POLLHUP at all. So do read() and check it return 0 must be the
>> only
>> condition to suspend source and open pipe for writing.
>
> Yes, I agree, see above, but I still don't understand why you stop polling.
>
>>
>> Also it cover cases when source suspended by user - we read data from pipe,
>> send resume, but source stays suspended. We must stop polling pipe, but
>> pollfd->events = 0 is not enough. If the writer make a open-write-close
>> while source stays suspended we will get endless POLLHUPs
>
> I think this could be covered differently. You can set a flag "is_suspended"
> when you have suspended the source because the writer disconnected.
> Then you can do something like
>
> if (SOURCE_IS_OPENED(source) || is_suspended) {
> if (!is_suspended && u->corkfd >= 0)
>close pipe for writing
>
> ...
>
> } else if (u->corkfd < 0)
> open pipe for writing
>
>
I need to stop polling to wait for source to be resumed if some bytes was
read and cannot be posted.
I can get POLLHUP spam if source is suspended, some data is waiting for source
to be resumed and writer closed pipe.
Example: source suspended by user and by autosuspend, pipe is waiting
for event (POLLIN), someone opened a pipe and send us data:
First iteration:
1. poll:
events = POLLIN
pa_rtpoll_run()
revents == POLLIN
2. read:
read() return 100
Re: [pulseaudio-discuss] [PATCH v7] pipe-source: implement autosuspend option
On 02/20/2018 04:18 PM, Georg Chini wrote: > On 20.02.2018 13:50, Raman Shishniou wrote: >> On 02/20/2018 03:16 PM, Georg Chini wrote: >>> On 20.02.2018 13:11, Georg Chini wrote: >>>> On 19.02.2018 16:01, Raman Shyshniou wrote: >>>>> Currently the pipe-source will remain running even if no >>>>> writer is connected and therefore no data is produced. >>>>> This patch adds the autosuspend= option to prevent this. >>>>> Source will stay suspended if no writer is connected. >>>>> This option is enabled by default. >>>>> --- >>>>> + >>>>> +pa_memchunk_reset(&chunk); >>>>> +chunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size); >>>> Further down, you might put some data in the memchunk before the >>>> read if the previous data was not frame aligned, therefore the memblock >>>> size must be u->pipe_size + fs. >>> Looking twice, I see you subtract the index from the read size. >>> Nevertheless I >>> guess it would be simpler to increase the memblock size by one frame. >> I prefer to keep buffer sizes to be power of 2. >> Currently pipe_size is 4096 (as determined by pulseaudio, actually 64k, >> I'll check later why). Adding one frame to the size of buffer makes it >> a little more than 4096 but much less then 8192> What is the advantage of >> keeping the buffer size a power of 2? > What if you have for example a channel count that is not a power > of 2? Would it not make more sense to keep the buffers frame > aligned? Anyway, I don't mind if you want to keep it your way. Usually memory is provided by pages, which is 4096 for x86/x86_64. Pipe size in linux is one page or 16 pages by default: http://man7.org/linux/man-pages/man7/pipe.7.html So the whole page will be used to store one extra frame which is 128 bytes (PA_CHANNELS_MAX * 4 bytes) maximum. I didn't read pa_memblock_new() code and may be it does not return page aligned memory. But I believe this is a good practice to keep page boundaries if possible. If you look again, the memblock always is frame aligned too. -- Raman ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v7] pipe-source: implement autosuspend option
On 02/20/2018 03:11 PM, Georg Chini wrote:
> On 19.02.2018 16:01, Raman Shyshniou wrote:
>> Currently the pipe-source will remain running even if no
>> writer is connected and therefore no data is produced.
>> This patch adds the autosuspend= option to prevent this.
>> Source will stay suspended if no writer is connected.
>> This option is enabled by default.
>> ---
>> src/modules/module-pipe-source.c | 279
>> ++-
>> 1 file changed, 215 insertions(+), 64 deletions(-)
>>
>> diff --git a/src/modules/module-pipe-source.c
>> b/src/modules/module-pipe-source.c
>> index f8284c1..c1a1e9c 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,30 @@ static int source_process_msg(
>> switch (code) {
>> +case PA_SOURCE_MESSAGE_SET_STATE:
>> +
>> +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();
>> +}
>> +
>> +break;
>> +
>> case PA_SOURCE_MESSAGE_GET_LATENCY: {
>> -size_t n = 0;
>> +int64_t latency;
>> +
>> +latency = PA_CLIP_SUB((int64_t)pa_rtclock_now(),
>> (int64_t)u->timestamp);
>> #ifdef FIONREAD
>> -int l;
>> +if (u->corkfd < 0) {
>> +int l;
>> -if (ioctl(u->fd, FIONREAD, &l) >= 0 && l > 0)
>> -n = (size_t) l;
>> +if (ioctl(u->fd, FIONREAD, &l) >= 0 && l > 0)
>> +latency += pa_bytes_to_usec((uint64_t)l,
>> &u->source->sample_spec);
>> +}
>> #endif
>>
>
> I don't think it is correct to add the data in the pipe to the latency.
> Essentially, the pipe should always be empty, because you read
> all data as soon as it is available. If a block of data is written to the
> pipe, you should already have accounted for that block through
> the time difference between now and the time stamp.
Yes. You are right. I'll drop this code.
>
>> -*((int64_t*) data) = pa_bytes_to_usec(n,
>> &u->source->sample_spec);
>> +*((int64_t*) data) = latency;
>> return 0;
>> }
>> }
>> @@ -121,7 +182,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(
Re: [pulseaudio-discuss] [PATCH v7] pipe-source: implement autosuspend option
On 02/20/2018 03:16 PM, Georg Chini wrote: > On 20.02.2018 13:11, Georg Chini wrote: >> On 19.02.2018 16:01, Raman Shyshniou wrote: >>> Currently the pipe-source will remain running even if no >>> writer is connected and therefore no data is produced. >>> This patch adds the autosuspend= option to prevent this. >>> Source will stay suspended if no writer is connected. >>> This option is enabled by default. >>> --- >>> + >>> +pa_memchunk_reset(&chunk); >>> +chunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size); >> >> Further down, you might put some data in the memchunk before the >> read if the previous data was not frame aligned, therefore the memblock >> size must be u->pipe_size + fs. > Looking twice, I see you subtract the index from the read size. Nevertheless I > guess it would be simpler to increase the memblock size by one frame. I prefer to keep buffer sizes to be power of 2. Currently pipe_size is 4096 (as determined by pulseaudio, actually 64k, I'll check later why). Adding one frame to the size of buffer makes it a little more than 4096 but much less then 8192. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Per (sink input, sink) volume control / routing
On 02/19/2018 06:46 PM, Andreas Hartmetz wrote: > Hello! > > I am currently developing an audio system for an automotive customer, > based on PulseAudio where PulseAudio is applicable. It is part of the > requirements that the volume of every input channel (sink input) to > every output channel (sink) can be configured separately. There are > good reasons for this, for example there can be two backseat info- > tainment "terminals" which could send their audio to a headphone for > each, or to the speakers in the whole car. The user-exposed volume > controls will of course present a simplified vew, but for factory > configuration, the described control is needed, and volume levels are > expected to change at runtime in response to some external events. > > The problem is that PulseAudio does not support that. It could almost > be "hacked" by using one module-combine-sink for every sink input, > which at least allows sending a stream to an arbitrary collection of > sinks. But it does not support volume control at all. > > So: > - Is there a way that I didn't think of to piece together the > desired volume control using existing modules and/or other > configuration? I don't want to resort to anything really weird > like creating lots of "loopback" devices using local sockets or > something like that (also for latency reasons), and running > multiple server instances also won't do it due to PA requiring to > open devices exclusively. Also, any hacky solution is not allowed > to significantly increase (or worse, hide from PA so it can't be > compensated) latency. > - If not, what do you think would be the best way to do it? I'm > thinking of a potential "module-crossbar" that I would need to > write, which allows matrix-style (n, m) volume control, i.e. > set_volume(sink_input_index, sink_index, volume). > Volume for each sink-input can configured separately: $ pactl list sink-inputs Sink Input #0 Driver: module-loopback.c Owner Module: 4 Client: n/a Sink: 0 Sample Specification: s16le 2ch 96000Hz Channel Map: front-left,front-right Format: pcm, format.sample_format = "\"s16le\"" format.rate = "96000" format.channels = "2" format.channel_map = "\"front-left,front-right\"" Corked: no Mute: no Volume: front-left: 65536 / 100% / 0.00 dB, front-right: 65536 / 100% / 0.00 dB balance 0.00 Buffer Latency: 133657 usec Sink Latency: 56936 usec Resample method: trivial Properties: media.role = "abstract" media.name = "Loopback from Null Input" media.icon_name = "audio-input-microphone" $ pactl set-sink-input-volume 0 32768 $ pactl list sink-inputs Sink Input #0 Driver: module-loopback.c Owner Module: 4 Client: n/a Sink: 0 Sample Specification: s16le 2ch 96000Hz Channel Map: front-left,front-right Format: pcm, format.sample_format = "\"s16le\"" format.rate = "96000" format.channels = "2" format.channel_map = "\"front-left,front-right\"" Corked: no Mute: no Volume: front-left: 32768 / 50% / -18.06 dB, front-right: 32768 / 50% / -18.06 dB balance 0.00 Buffer Latency: 133593 usec Sink Latency: 20936 usec Resample method: trivial Properties: media.role = "abstract" media.name = "Loopback from Null Input" media.icon_name = "audio-input-microphone" > Thanks in advance! > > Andreas Hartmetz > > > ___ > pulseaudio-discuss mailing list > pulseaudio-discuss@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss > -- Raman ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] loopback module double resampling
Hello, Currently a loopback module does a double resampling if the source and sink sample rates are different. I used a simple config to test it: load-module module-null-source source_name=src rate=48000 load-module module-null-sink sink_name=dst rate=96000 load-module module-loopback source=src sink=dst After start I see in logs: D: [pulseaudio] module-loopback.c: Loopback overall latency is 56.14 ms + 67.48 ms + 53.20 ms = 176.76 ms D: [pulseaudio] module-loopback.c: Loopback latency at base rate is 176.46 ms D: [pulseaudio] module-loopback.c: [dst] Updated sampling rate to 95758 Hz. D: [pulseaudio] module-loopback.c: Loopback overall latency is 23.17 ms + 134.20 ms + 44.60 ms = 201.94 ms D: [pulseaudio] module-loopback.c: Loopback latency at base rate is 201.60 ms D: [pulseaudio] module-loopback.c: [dst] Updated sampling rate to 95925 Hz. D: [pulseaudio] module-loopback.c: Loopback overall latency is 44.15 ms + 133.80 ms + 31.56 ms = 209.48 ms D: [pulseaudio] module-loopback.c: Loopback latency at base rate is 209.37 ms D: [pulseaudio] module-loopback.c: [dst] Updated sampling rate to 95994 Hz. Sink-inputs and source-outputs: $ ~/bin/pactl list short source-outputs 0 0 - module-loopback.c s16le 2ch 96000Hz $ ~/bin/pactl list short sink-inputs 0 0 - module-loopback.c s16le 2ch 95994Hz I.e. first resample occurs in source-output (48000Hz -> 96000Hz) and the second in sink-input (95994Hz -> 96000Hz). I think it should change source-output's sample rate during latency recalculations. In this case there should be only one real resampler. This should increase audio quality and reduce cpu usage. -- Raman ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v6 1/2] pipe-source: generate silence when no writers connected
On 02/16/2018 12:00 PM, Georg Chini wrote:
> On 14.02.2018 23:16, Raman Shyshniou wrote:
>> Currently the pipe-source does not produce any data if no
>> writer is connected. This patch enable silence generator
>> when last writer closed pipe. It will stop automatically
>> when any data appears.
>> ---
> After my fixes to module-null-source, I think your logic is not yet
> completely correct. I would propose to do it like that:
>
> In source_process_msg():
>
> case PA_SOURCE_MESSAGE_GET_LATENCY:
>current_latency = now - time stamp
>if (u->corkfd < 0)
>current_latency += data in pipe
>
> case PA_SOURCE_MESSAGE_SET_STATE:
>if (SUSPENDED or INIT -> IDLE || SUSPENDED or INIT -> RUNNING)
> {
>get time stamp
>u->starting = true
>}
>
> In thread_func():
>
> close u->corkfd
> u->corkfd = -1
> u->starting = true
>
> timer_elapsed = false
> revents = 0
> get time stamp
>
> for (;;) {
> if (source is open) {
>
> /* We have to wait at least one configured source latency before
> starting
> * to read data */
> if (revents & POLLIN && !u->starting) {
> read data from pipe
> if (u->corkfd >=0) {
> close corkfd
> u->corkfd = -1
>}
>
> } else if (timer_elapsed && u->corkfd > 0)
> generate silence
>
>if (data was read/generated) {
> post data
> time stamp += data written
>}
>
>set timer absolute time stamp + configured (or fixed) source latency
>} else
>set timer disabled
>
>run rtpoll
>get timer_elapsed
>
> if (u->starting && timer_elapsed)
> u->starting = false
>
>if (revents & POLLHUP) {
>open pipe for writing
>u->corkfd = write file descriptor
>revents = revents & ~POLLHUP
>}
>
>error check
> }
>
> You can also add a source_update_requested_latency_cb() like
> in module-null-source and pass PA_SOURCE_DYNAMIC_LATENCY
> to pa_source_new() to make the latency configurable.
>
> I hope I did not forget anything ...
The incoming data can has a sample rate that differs from the system clock.
For example, due to imperfect hardware oscillator. It's a bad idea to expect
a data at the system clock rate. When the source is receiving a real data from
pipe the timer should be disabled and u->timestamp and u->starting doesn't make
sense.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v6 1/2] pipe-source: generate silence when no writers connected
On 02/16/2018 12:00 PM, Georg Chini wrote:
> On 14.02.2018 23:16, Raman Shyshniou wrote:
>> Currently the pipe-source does not produce any data if no
>> writer is connected. This patch enable silence generator
>> when last writer closed pipe. It will stop automatically
>> when any data appears.
>> ---
> After my fixes to module-null-source, I think your logic is not yet
> completely correct. I would propose to do it like that:
>
> In source_process_msg():
>
> case PA_SOURCE_MESSAGE_GET_LATENCY:
>current_latency = now - time stamp
>if (u->corkfd < 0)
>current_latency += data in pipe
>
> case PA_SOURCE_MESSAGE_SET_STATE:
>if (SUSPENDED or INIT -> IDLE || SUSPENDED or INIT -> RUNNING)
> {
>get time stamp
>u->starting = true
>}
>
> In thread_func():
>
> close u->corkfd
> u->corkfd = -1
> u->starting = true
>
> timer_elapsed = false
> revents = 0
> get time stamp
>
> for (;;) {
> if (source is open) {
>
> /* We have to wait at least one configured source latency before
> starting
> * to read data */
> if (revents & POLLIN && !u->starting) {
> read data from pipe
> if (u->corkfd >=0) {
> close corkfd
> u->corkfd = -1
>}
>
> } else if (timer_elapsed && u->corkfd > 0)
> generate silence
>
>if (data was read/generated) {
> post data
> time stamp += data written
>}
>
>set timer absolute time stamp + configured (or fixed) source latency
>} else
>set timer disabled
>
>run rtpoll
>get timer_elapsed
>
> if (u->starting && timer_elapsed)
> u->starting = false
>
>if (revents & POLLHUP) {
>open pipe for writing
>u->corkfd = write file descriptor
>revents = revents & ~POLLHUP
>}
>
Unfortunately not all platforms generate POLLHUP when writer closed a pipe:
Moreover, some platforms generate POLLIN with or without POLLHUP
https://www.greenend.org.uk/rjk/tech/poll.html
So, the only correct way is try to read EOF (0) from pipe when POLLIN or
POLLHUP in revents.
>error check
> }
>
> You can also add a source_update_requested_latency_cb() like
> in module-null-source and pass PA_SOURCE_DYNAMIC_LATENCY
> to pa_source_new() to make the latency configurable.
The pipe-source can change latency only when it generating a silence.
>
> I hope I did not forget anything ...
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] null-source: fix multiple bugs
On 02/15/2018 11:51 PM, Georg Chini wrote:
> The current null-source implementation has several bugs:
>
> 1) The latency reported is the negative of the correct latency.
> 2) The memchunk passed to pa_source_post() is not initialized
> with silence.
> 3) In PA_SOURCE_MESSAGE_SET_STATE the timestamp is always set
> when the source transitions to RUNNING state. This should only
> happen when the source transitions from SUSPENDED to RUNNING
> but also if it changes from SUSPENDED to IDLE.
> 4) The timing of the thread function is incorrect. It always
> uses u->latency_time, regardless of the specified source
> latency.
> 5) The latency_time argument seems pointless because the source
> is defined with dynamic latency.
>
> This patch fixes the issues by
> 1) inverting the sign of the reported latency,
> 2) initializing the memchunk with silence,
> 3) changing the logic in PA_SOURCE_MESSAGE_SET_STATE so that
> the timestamp is set when needed,
> 4) using u->block_usec instead of u->latency_time for setting
> the rtpoll timer and checking if the timer has elapsed,
> 5) removing the latency_time option.
> ---
> src/modules/module-null-source.c | 40
> +++-
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/src/modules/module-null-source.c
> b/src/modules/module-null-source.c
> index 41f17bd9..7bbe7f47 100644
> --- a/src/modules/module-null-source.c
> +++ b/src/modules/module-null-source.c
> @@ -51,12 +51,11 @@ PA_MODULE_USAGE(
> "rate= "
> "source_name= "
> "channel_map= "
> -"description= "
> -"latency_time=");
> +"description= ");
>
> #define DEFAULT_SOURCE_NAME "source.null"
> -#define DEFAULT_LATENCY_TIME 20
> #define MAX_LATENCY_USEC (PA_USEC_PER_SEC * 2)
> +#define MIN_LATENCY_USEC (500)
>
> struct userdata {
> pa_core *core;
> @@ -71,7 +70,6 @@ struct userdata {
>
> pa_usec_t block_usec;
> pa_usec_t timestamp;
> -pa_usec_t latency_time;
> };
>
> static const char* const valid_modargs[] = {
> @@ -81,7 +79,6 @@ static const char* const valid_modargs[] = {
> "source_name",
> "channel_map",
> "description",
> -"latency_time",
> NULL
> };
>
> @@ -91,8 +88,10 @@ static int source_process_msg(pa_msgobject *o, int code,
> void *data, int64_t off
> switch (code) {
> case PA_SOURCE_MESSAGE_SET_STATE:
>
> -if (PA_PTR_TO_UINT(data) == PA_SOURCE_RUNNING)
> -u->timestamp = pa_rtclock_now();
> +if (pa_source_get_state(u->source) == PA_SOURCE_SUSPENDED ||
> pa_source_get_state(u->source) == PA_SOURCE_INIT) {
> +if (PA_PTR_TO_UINT(data) == PA_SOURCE_RUNNING ||
> PA_PTR_TO_UINT(data) == PA_SOURCE_IDLE)
> +u->timestamp = pa_rtclock_now();
> +}
pa_source_get_state() is the macro:
#define pa_source_get_state(s) ((pa_source_state_t) (s)->state)
I think it's unsafe to access u->source->state in source_process_msg() since it
called from i/o thread context.
Also there is the macro PA_SOURCE_IS_OPENED(state) which check the source is
running or idle.
I think it should look like this:
-if (PA_PTR_TO_UINT(data) == PA_SOURCE_RUNNING)
-u->timestamp = pa_rtclock_now();
+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();
+}
>
> break;
>
> @@ -100,7 +99,7 @@ static int source_process_msg(pa_msgobject *o, int code,
> void *data, int64_t off
> pa_usec_t now;
>
> now = pa_rtclock_now();
> -*((int64_t*) data) = (int64_t)u->timestamp - (int64_t)now;
> +*((int64_t*) data) = (int64_t)now - (int64_t)u->timestamp;
>
> return 0;
> }
> @@ -117,10 +116,13 @@ static void
> source_update_requested_latency_cb(pa_source *s) {
> pa_assert(u);
>
> u->block_usec = pa_source_get_requested_latency_within_thread(s);
> +if (u->block_usec == (pa_usec_t)-1)
> +u->block_usec = u->source->thread_info.max_latency;
> }
>
> static void thread_func(void *userdata) {
> struct userdata *u = userdata;
> +bool timer_elapsed = false;
>
> pa_assert(u);
>
> @@ -140,17 +142,19 @@ static void thread_func(void *userdata) {
>
> now = pa_rtclock_now();
>
> -if ((chunk.length = pa_usec_to_bytes(now - u->timestamp,
> &u->source->sample_spec)) > 0) {
> +if (timer_elapsed && (chunk.length = pa_usec_to_bytes(now -
> u->timestamp, &u->source->sample_spec)) > 0) {
>
> -chunk.memblock = pa_memblock_new(u->core->mempool, (size_t)
> -1); /* or chunk.length? */
> +chunk.length =
> PA_MIN(pa_mempool_block_size_max(u->core->mempool), chunk.length);
> +chunk.
Re: [pulseaudio-discuss] [PATCH] loopback: underrun_protection argument
On 02/15/2018 12:43 PM, Georg Chini wrote: > On 15.02.2018 10:31, Raman Shishniou wrote: >> On 02/15/2018 10:20 AM, Georg Chini wrote: >>> On 14.02.2018 22:51, Raman Shishniou wrote: >>>> On 02/14/2018 07:20 PM, Georg Chini wrote: >>>>> On 14.02.2018 15:25, Raman Shyshniou wrote: >>>>>> This patch adds a underrun_protection argument to >>>>>> control underrun protection algorithm. Disabling >>>>>> protection will keep loopback latency regardless >>>>>> of underruns. >>>>> Again I do not understand the motivation of the patch. >>>>> In what situations are you seeing so many underruns and >>>>> still want to keep the original configured latency value? >>>>> Audio will be very bad in that case. >>>>> >>>> All situations where where latency is more important than data integrity. >>>> Voice over IP (telephony) for example, receiving audio data using network >>>> by UDP/RTP. Any data loss leads to underruns in loopback module. >>> This is not correct. It will only lead to underruns, if module-loopback runs >>> out of data. So if you buffer enough data, missing packets in the voice >>> stream would just appear to be small sample rate variations from the >>> perspective of the loopback module. Because the loopback module >>> does some adaptive re-sampling, these variations are no problem. >>> >>> Maybe this happens for you because module-pipe-source has no buffer >>> at all and simply passes the values through whenever data arrives. >>> With missing data packets, this can surely lead to underruns on the >>> source side which are passed on to the loopback module. >>> Perhaps you should implement some buffering in pipe-source? >>> >>> I don't see the point of your change. If you are seeing massive underruns, >>> audio quality will be really bad and just sticking to the configured latency >>> will not improve the situation. For me, the permanent occurrence of >>> underruns shows that there is something wrong with your setup in the first >>> place. The better idea is to correct the audio chain, so that no (or only >>> very >>> few) underruns happen. >> I Agree. For live streaming or internet radio I can buffer more data up to >> several seconds (or just use tcp). But not for telephony, where 10-20 >> underruns per hour is acceptable, but latency more than 50-100ms is not. >> Loopback module increases latency permanently. There is no way to decrease >> it without unload and load again. > > How about a max_latency_msec argument in the sense that the logic > will not be completely disabled, but module-loopback will not try to > increase the latency above max_latency_msec (if specified) even if > underruns occur? > I can see that, it makes sense to me. I'll make the patch. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] loopback: underrun_protection argument
On 02/15/2018 10:20 AM, Georg Chini wrote: > On 14.02.2018 22:51, Raman Shishniou wrote: >> On 02/14/2018 07:20 PM, Georg Chini wrote: >>> On 14.02.2018 15:25, Raman Shyshniou wrote: >>>> This patch adds a underrun_protection argument to >>>> control underrun protection algorithm. Disabling >>>> protection will keep loopback latency regardless >>>> of underruns. >>> Again I do not understand the motivation of the patch. >>> In what situations are you seeing so many underruns and >>> still want to keep the original configured latency value? >>> Audio will be very bad in that case. >>> >> All situations where where latency is more important than data integrity. >> Voice over IP (telephony) for example, receiving audio data using network >> by UDP/RTP. Any data loss leads to underruns in loopback module. > > This is not correct. It will only lead to underruns, if module-loopback runs > out of data. So if you buffer enough data, missing packets in the voice > stream would just appear to be small sample rate variations from the > perspective of the loopback module. Because the loopback module > does some adaptive re-sampling, these variations are no problem. > > Maybe this happens for you because module-pipe-source has no buffer > at all and simply passes the values through whenever data arrives. > With missing data packets, this can surely lead to underruns on the > source side which are passed on to the loopback module. > Perhaps you should implement some buffering in pipe-source? > > I don't see the point of your change. If you are seeing massive underruns, > audio quality will be really bad and just sticking to the configured latency > will not improve the situation. For me, the permanent occurrence of > underruns shows that there is something wrong with your setup in the first > place. The better idea is to correct the audio chain, so that no (or only very > few) underruns happen. I Agree. For live streaming or internet radio I can buffer more data up to several seconds (or just use tcp). But not for telephony, where 10-20 underruns per hour is acceptable, but latency more than 50-100ms is not. Loopback module increases latency permanently. There is no way to decrease it without unload and load again. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] null-source: correct latency report and silence memory
On 02/15/2018 10:22 AM, Georg Chini wrote:
> On 14.02.2018 23:09, Raman Shishniou wrote:
>> On 02/15/2018 12:03 AM, Georg Chini wrote:
>>> The null-source currently reports the negative of the correct latency.
>>> Also the memchunk passed to pa_source_post() is not initialized with
>>> silence.
>>>
>>> This patch fixes both issues.
>>> ---
>>> src/modules/module-null-source.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/modules/module-null-source.c
>>> b/src/modules/module-null-source.c
>>> index 41f17bd9..6310bda9 100644
>>> --- a/src/modules/module-null-source.c
>>> +++ b/src/modules/module-null-source.c
>>> @@ -100,7 +100,7 @@ static int source_process_msg(pa_msgobject *o, int
>>> code, void *data, int64_t off
>>> pa_usec_t now;
>>> now = pa_rtclock_now();
>>> -*((int64_t*) data) = (int64_t)u->timestamp - (int64_t)now;
>>> +*((int64_t*) data) = (int64_t)now - (int64_t)u->timestamp;
>>> return 0;
>>> }
>>> @@ -142,8 +142,9 @@ static void thread_func(void *userdata) {
>>> if ((chunk.length = pa_usec_to_bytes(now - u->timestamp,
>>> &u->source->sample_spec)) > 0) {
>>> -chunk.memblock = pa_memblock_new(u->core->mempool,
>>> (size_t) -1); /* or chunk.length? */
>>> +chunk.memblock = pa_memblock_new(u->core->mempool,
>>> chunk.length);
>>> chunk.index = 0;
>>> +pa_silence_memchunk(&chunk, &u->source->sample_spec);
>>> pa_source_post(u->source, &chunk);
>>> pa_memblock_unref(chunk.memblock);
>> I think you need to change the next line too:
>> -u->timestamp = now;
>> +u->timestamp += pa_bytes_to_usec(chunk.length,
>> &u->source->sample_spec);
>> }
>>
>> to make silence generator more stable
>>
>>
> How would that make it more stable? It will only lower the precision of the
> time stamp.
Nope. It will increase the precision. Actually null-source send
pa_bytes_to_usec(chunk.length)
microseconds of data each iteration which can be smaller than (now -
u->timestamp).
It must increase timestamp by actually data sent. Accumulated difference will
be corrected
on the next iteration by sending more data.
For example each 10ms we need to send a 99.5 bytes of data (s8, mono, 9950Hz),
i.e. pa_usec_to_bytes() does: bytes = usecs * 0.00995
and pa_bytes_to_usec() does: usecs = bytes / 0.00995
First iteration:
u->timestamp == 0
now = pa_rtclock_now() == 10ms == 1us
bytes = pa_usec_to_bytes(1 - 0) = 99
... send 99 bytes of data ...
u->timestamp += pa_bytes_to_usec(99) == 9949
Second iteration:
u->timestamp == 9949
now = pa_rtclock_now() == 20ms == 2us
bytes = pa_usec_to_bytes(2 - 9949) = 100
... send 100 bytes of data ...
u->timestamp += pa_bytes_to_used(100) = 9949 + 10050 = 1
Third iteration:
u->timestamp == 1
now = pa_rtclock_now() == 30ms == 3us
bytes = pa_usec_to_bytes(3 - 1) = 99
... send 99 bytes of data ...
u->timestamp += pa_bytes_to_used(99) = 1 + 9949 = 29948
and so on
Current implementation will send only 99 bytes of data each iteration.
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] null-source: correct latency report and silence memory
On 02/15/2018 12:03 AM, Georg Chini wrote:
> The null-source currently reports the negative of the correct latency.
> Also the memchunk passed to pa_source_post() is not initialized with
> silence.
>
> This patch fixes both issues.
> ---
> src/modules/module-null-source.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/modules/module-null-source.c
> b/src/modules/module-null-source.c
> index 41f17bd9..6310bda9 100644
> --- a/src/modules/module-null-source.c
> +++ b/src/modules/module-null-source.c
> @@ -100,7 +100,7 @@ static int source_process_msg(pa_msgobject *o, int code,
> void *data, int64_t off
> pa_usec_t now;
>
> now = pa_rtclock_now();
> -*((int64_t*) data) = (int64_t)u->timestamp - (int64_t)now;
> +*((int64_t*) data) = (int64_t)now - (int64_t)u->timestamp;
>
> return 0;
> }
> @@ -142,8 +142,9 @@ static void thread_func(void *userdata) {
>
> if ((chunk.length = pa_usec_to_bytes(now - u->timestamp,
> &u->source->sample_spec)) > 0) {
>
> -chunk.memblock = pa_memblock_new(u->core->mempool, (size_t)
> -1); /* or chunk.length? */
> +chunk.memblock = pa_memblock_new(u->core->mempool,
> chunk.length);
> chunk.index = 0;
> +pa_silence_memchunk(&chunk, &u->source->sample_spec);
> pa_source_post(u->source, &chunk);
> pa_memblock_unref(chunk.memblock);
I think you need to change the next line too:
-u->timestamp = now;
+u->timestamp += pa_bytes_to_usec(chunk.length,
&u->source->sample_spec);
}
to make silence generator more stable
>
>
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] loopback: underrun_protection argument
On 02/14/2018 07:20 PM, Georg Chini wrote:
> On 14.02.2018 15:25, Raman Shyshniou wrote:
>> This patch adds a underrun_protection argument to
>> control underrun protection algorithm. Disabling
>> protection will keep loopback latency regardless
>> of underruns.
>
> Again I do not understand the motivation of the patch.
> In what situations are you seeing so many underruns and
> still want to keep the original configured latency value?
> Audio will be very bad in that case.
>
All situations where where latency is more important than data integrity.
Voice over IP (telephony) for example, receiving audio data using network
by UDP/RTP. Any data loss leads to underruns in loopback module. Current
implementation will increase overall latency by 5 msec every 3 underruns.
>> ---
>> src/modules/module-loopback.c | 31 +--
>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
>> index aecac0a..c452e1a 100644
>> --- a/src/modules/module-loopback.c
>> +++ b/src/modules/module-loopback.c
>> @@ -45,6 +45,7 @@ PA_MODULE_USAGE(
>> "sink= "
>> "adjust_time= "
>> "latency_msec= "
>> +"underrun_protection= "
>> "format= "
>> "rate= "
>> "channels= "
>> @@ -90,6 +91,7 @@ struct userdata {
>> /* Values from command line configuration */
>> pa_usec_t latency;
>> pa_usec_t adjust_time;
>> +bool underrun_protection;
>> /* Latency boundaries and current values */
>> pa_usec_t min_source_latency;
>> @@ -158,6 +160,7 @@ static const char* const valid_modargs[] = {
>> "sink",
>> "adjust_time",
>> "latency_msec",
>> +"underrun_protection",
>> "format",
>> "rate",
>> "channels",
>> @@ -325,11 +328,20 @@ static void adjust_rates(struct userdata *u) {
>> /* If we are seeing underruns then the latency is too small */
>> if (u->underrun_counter > 2) {
>> -u->underrun_latency_limit = PA_MAX(u->latency, u->minimum_latency)
>> + 5 * PA_USEC_PER_MSEC;
>> -u->underrun_latency_limit =
>> PA_CLIP_SUB((int64_t)u->underrun_latency_limit, u->sink_latency_offset +
>> u->source_latency_offset);
>> -update_minimum_latency(u, u->sink_input->sink, false);
>> -pa_log_warn("Too many underruns, increasing latency to %0.2f ms",
>> (double)u->minimum_latency / PA_USEC_PER_MSEC);
>> -u->underrun_counter = 0;
>> +int64_t target_latency;
>> +
>> +target_latency = PA_MAX(u->latency, u->minimum_latency);
>> +
>> +if (u->underrun_protection)
>> +target_latency += 5 * PA_USEC_PER_MSEC;
>> +
>> +u->underrun_latency_limit = PA_CLIP_SUB(target_latency,
>> u->sink_latency_offset + u->source_latency_offset);
>> +
>> +if (u->minimum_latency < u->underrun_latency_limit) {
>
> The above comparison does not make sense to me. The minimum latency
> should always be smaller than (or equal to) the underrun_latency_limit.
> Otherwise we would not be seeing underruns. There are two ways, the
> minimum latency is calculated:
> 1) Theoretically. Anything below that value will definitely cause underruns.
> The value is directly derived from the minimum sink/source latency.
> 2) Practically: If we are seeing underruns at some latency, we know the
> theoretical value is too small and update it to the empirically found value.
>
>> +update_minimum_latency(u, u->sink_input->sink, false);
>> +pa_log_warn("Too many underruns, increasing latency to %0.2f
>> ms", (double)u->minimum_latency / PA_USEC_PER_MSEC);
>> +u->underrun_counter = 0;
>> +}
>> }
>> /* Allow one underrun per hour */
>> @@ -347,7 +359,7 @@ static void adjust_rates(struct userdata *u) {
>> }
>> u->adjust_time_stamp = now;
>> -/* Rates and latencies*/
>> +/* Rates and latencies */
>> old_rate = u->sink_input->sample_spec.rate;
>> base_rate = u->source_output->sample_spec.rate;
>> @@ -1251,6 +1263,7 @@ int pa__init(pa_module *m) {
>> pa_source *source = NULL;
>> pa_source_output_new_data source_output_data;
>> bool source_dont_move;
>> +bool underrun_protection = true;
>> uint32_t latency_msec;
>> pa_sample_spec ss;
>> pa_channel_map map;
>> @@ -1336,10 +1349,16 @@ int pa__init(pa_module *m) {
>> goto fail;
>> }
>> +if (pa_modargs_get_value_boolean(ma, "underrun_protection",
>> &underrun_protection) < 0) {
>> +pa_log("Failed to parse underrun_protection value.");
>> +goto fail;
>> +}
>> +
>> m->userdata = u = pa_xnew0(struct userdata, 1);
>> u->core = m->core;
>> u->module = m;
>> u->latency = (pa_usec_t) latency_msec * PA_USEC_PER_MSEC;
>> +u->underrun_protection = underrun_protection;
>> u->output_thread_info.pop_called = false;
>> u->output_thr
Re: [pulseaudio-discuss] [PATCH v3] pipe-source: optional suspend source when no writers connected to fifo
On 02/13/2018 06:37 PM, Georg Chini wrote:
> On 11.02.2018 00:36, Raman Shyshniou wrote:
>> Add autosuspend= option. Enabling this option will make
>> pipe-source suspended when all writers closed fifo. Source will
>> be automatically unsuspended if any data will be written to pipe
>> and suspended again when last writer closed fifo.
> Can you add a sentence about the motivation to the commit
> message? Something like "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. ..."
>
Ok.
> Otherwise only two small comments below.
>> ---
>> src/modules/module-pipe-source.c | 136
>> +--
>> 1 file changed, 129 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/modules/module-pipe-source.c
>> b/src/modules/module-pipe-source.c
>> index f8284c1..af7badc 100644
>> --- a/src/modules/module-pipe-source.c
>> +++ b/src/modules/module-pipe-source.c
>> @@ -57,11 +57,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,7 +84,10 @@ struct userdata {
>> pa_thread_mq thread_mq;
>> pa_rtpoll *rtpoll;
>> +pipe_source_msg *msg;
>> +
>> char *filename;
>> +int corkfd;
>> int fd;
>> pa_memchunk memchunk;
>> @@ -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 because writer connected",
>> 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,
>> @@ -135,8 +183,15 @@ static void thread_func(void *userdata) {
>> pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>> +/* Writer connected */
>> +if (u->corkfd >= 0 && pollfd->revents & POLLIN) {
>> +pa_asyncmsgq_post(pa_thread_mq_get()->outq,
>> PA_MSGOBJECT(u->msg), PIPE_SOURCE_RESUME, u, 0, NULL, NULL);
>> +pa_assert_se(pa_close(u->corkfd) == 0);
>> +u->corkfd = -1;
>> +}
>> +
>> /* Try to read some data and pass it on to the source driver */
>> -if (u->source->thread_info.state == PA_SOURCE_RUNNING &&
>> pollfd->revents) {
>> +if (u->source->thread_info.state == PA_SOURCE_RUNNING &&
>> pollfd->revents & POLLIN) {
>> ssize_t l;
>> void *p;
>> @@ -151,8 +206,6 @@ static void thread_func(void *userdata) {
>> l = pa_read(u->fd, (uint8_t*) p + u->memchunk.index,
>> pa_memblock_get_length(u->memchunk.memblock) - u->memchunk.index,
>> &read_type);
>> pa_memblock_release(u->memchunk.memblock);
>> -pa_assert(l != 0); /* EOF cannot happen, since we opened the
>> fifo for both reading and writing */
>> -
>> if (l < 0) {
>> if (errno == EINTR)
>> @@ -162,6 +215,11 @@ static void thread_func(void *userdata) {
>> goto fail;
>> }
>> +} else if (l == 0) {
>> +
>> +/* Nothing to read */
>> +pollfd->revents = 0;
>> +
>> } else {
>> u->memchunk.length = (size_t) l;
>> @@ -178,7 +236,7 @@ static void thread_func(void *userdata) {
>> }
>> /* Hmm, nothing to do. Let's sleep */
>> -pollfd->events = (short) (u->source->thread_info.state ==
>> PA_SOURCE_RUNNING ? POLLIN : 0);
>> +pollfd->events = (short) ((u
Re: [pulseaudio-discuss] [PATCH v2] pipe-source: suspend source when no writers connected to fifo
On 02/13/2018 06:41 PM, Georg Chini wrote: > On 13.02.2018 11:27, Raman Shishniou wrote: >> On 02/13/2018 11:58 AM, Georg Chini wrote: >>> On 12.02.2018 17:23, Tanu Kaskinen wrote: >>>> On Sat, 2018-02-10 at 23:08 +0100, Georg Chini wrote: >>>>> On 10.02.2018 23:04, Raman Shishniou wrote: >>>>>> On 02/11/2018 12:43 AM, Georg Chini wrote: >>>>>>> On 10.02.2018 22:25, Raman Shuishniou wrote: >>>>>>>> 10.02.2018 23:59, Georg Chini пишет: >>>>>>>>> On 08.02.2018 17:58, Raman Shyshniou wrote: >>>>>>>>>> Make pipe-source suspended if all writers closed fifo. >>>>>>>>>> Source will be automatically unsuspended if any data will >>>>>>>>>> be written to pipe and suspended again when last writer >>>>>>>>>> closed fifo. >>>>>>>>>> --- >>>>>>>>>> src/modules/module-pipe-source.c | 114 >>>>>>>>>> +-- >>>>>>>>>> 1 file changed, 109 insertions(+), 5 deletions(-) >>>>>>>>>> >>>>>>>>> What is the motivation/use case for the patch? Normally, if there >>>>>>>>> are no writers, the source should deliver silence like any other >>>>>>>>> source without input signal. >>>>>>>> pipe-source module can't generate silence if no writers connected >>>>>>>> because it has no clock. Aactually incoming data has some clock, but >>>>>>>> not pipe-source itself. Use the system clock to generate silence >>>>>>>> is a bad idea because it may differ from incoming data clock. >>>>>>> Take a look at the recent patches for pipe-sink that introduced using >>>>>>> system-clock timing. I guess something similar should be possible >>>>>>> for the pipe-source. Why would it be a problem, if the timing of the >>>>>>> silence is slightly different from that of the writer? Different writers >>>>>>> may have different timing anyway, so if one writer disconnects >>>>>>> and another connects, timing may change. >>>>>>> module-loopback is able to deal with sample rate changes on the >>>>>>> input side and will adapt the sample rate of the output side so that >>>>>>> it matches the incoming rate to keep a constant latency. >>>>>>> >>>>>> I seen the last patches for pipe-sink module. I think there is no >>>>>> reason to generate silence in pipe-source module with system clock. >>>>>> The source outputs will do read zeros (resample, convert) - just a >>>>>> waste of cpu time. >>>>>> >>>>> I see your point. The reason for generating silence would >>>>> be to have consistent behavior for all sources. I'll ask Tanu >>>>> for his opinion. >>>> My opinion: it would be nice to generate silence by default if someone >>>> is willing to implement that, but suspending is better than the current >>>> behaviour, if the current behaviour is to have the source state as >>>> RUNNING while not producing any data. >>>> >>> Raman, are you willing to implement generating silence? This >>> could also cover the case where a writer stays connected but >>> does not provide any data. >>> >>> If not, I will proceed reviewing your patch. >>> >> Ok. I'll implement silence generator, but I want to leave an autosuspend >> option. >> > Fine for me to keep the autosuspend option. I guess the "silence generator" > will then be another patch on top of this patch? > No. I make a new patch with both silence generator and autosuspend options. ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2] pipe-source: suspend source when no writers connected to fifo
On 02/13/2018 11:58 AM, Georg Chini wrote: > On 12.02.2018 17:23, Tanu Kaskinen wrote: >> On Sat, 2018-02-10 at 23:08 +0100, Georg Chini wrote: >>> On 10.02.2018 23:04, Raman Shishniou wrote: >>>> On 02/11/2018 12:43 AM, Georg Chini wrote: >>>>> On 10.02.2018 22:25, Raman Shuishniou wrote: >>>>>> 10.02.2018 23:59, Georg Chini пишет: >>>>>>> On 08.02.2018 17:58, Raman Shyshniou wrote: >>>>>>>>Make pipe-source suspended if all writers closed fifo. >>>>>>>>Source will be automatically unsuspended if any data will >>>>>>>>be written to pipe and suspended again when last writer >>>>>>>>closed fifo. >>>>>>>> --- >>>>>>>> src/modules/module-pipe-source.c | 114 >>>>>>>> +-- >>>>>>>> 1 file changed, 109 insertions(+), 5 deletions(-) >>>>>>>> >>>>>>> What is the motivation/use case for the patch? Normally, if there >>>>>>> are no writers, the source should deliver silence like any other >>>>>>> source without input signal. >>>>>> pipe-source module can't generate silence if no writers connected >>>>>> because it has no clock. Aactually incoming data has some clock, but >>>>>> not pipe-source itself. Use the system clock to generate silence >>>>>> is a bad idea because it may differ from incoming data clock. >>>>> Take a look at the recent patches for pipe-sink that introduced using >>>>> system-clock timing. I guess something similar should be possible >>>>> for the pipe-source. Why would it be a problem, if the timing of the >>>>> silence is slightly different from that of the writer? Different writers >>>>> may have different timing anyway, so if one writer disconnects >>>>> and another connects, timing may change. >>>>> module-loopback is able to deal with sample rate changes on the >>>>> input side and will adapt the sample rate of the output side so that >>>>> it matches the incoming rate to keep a constant latency. >>>>> >>>> I seen the last patches for pipe-sink module. I think there is no >>>> reason to generate silence in pipe-source module with system clock. >>>> The source outputs will do read zeros (resample, convert) - just a >>>> waste of cpu time. >>>> >>> I see your point. The reason for generating silence would >>> be to have consistent behavior for all sources. I'll ask Tanu >>> for his opinion. >> My opinion: it would be nice to generate silence by default if someone >> is willing to implement that, but suspending is better than the current >> behaviour, if the current behaviour is to have the source state as >> RUNNING while not producing any data. >> > Raman, are you willing to implement generating silence? This > could also cover the case where a writer stays connected but > does not provide any data. > > If not, I will proceed reviewing your patch. > Ok. I'll implement silence generator, but I want to leave an autosuspend option. -- Raman ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2] pipe-source: suspend source when no writers connected to fifo
On 02/11/2018 12:43 AM, Georg Chini wrote: > On 10.02.2018 22:25, Raman Shuishniou wrote: >> 10.02.2018 23:59, Georg Chini пишет: >>> On 08.02.2018 17:58, Raman Shyshniou wrote: Make pipe-source suspended if all writers closed fifo. Source will be automatically unsuspended if any data will be written to pipe and suspended again when last writer closed fifo. --- src/modules/module-pipe-source.c | 114 +-- 1 file changed, 109 insertions(+), 5 deletions(-) >>> What is the motivation/use case for the patch? Normally, if there >>> are no writers, the source should deliver silence like any other >>> source without input signal. >> >> pipe-source module can't generate silence if no writers connected >> because it has no clock. Aactually incoming data has some clock, but >> not pipe-source itself. Use the system clock to generate silence >> is a bad idea because it may differ from incoming data clock. > > Take a look at the recent patches for pipe-sink that introduced using > system-clock timing. I guess something similar should be possible > for the pipe-source. Why would it be a problem, if the timing of the > silence is slightly different from that of the writer? Different writers > may have different timing anyway, so if one writer disconnects > and another connects, timing may change. > module-loopback is able to deal with sample rate changes on the > input side and will adapt the sample rate of the output side so that > it matches the incoming rate to keep a constant latency. > I seen the last patches for pipe-sink module. I think there is no reason to generate silence in pipe-source module with system clock. The source outputs will do read zeros (resample, convert) - just a waste of cpu time. >> >> For example the loopback module connected to pipe-source without >> writers just spams messages like this: >> [alsa-sink-USB Audio] module-loopback.c: Could not peek into queue >> >> I can make autosuspend behaviour optional. > Making it optional is a good idea in any case. I'll make a v3 version with option to enable autosuspend behaviour ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
