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 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.
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.
BTW, currently pipe-source PA just crashed if I try to write s24le to pipe:
$ cat ~/.config/pulse/default.pa
#!/usr/bin/pulseaudio -nF
load-module module-cli-protocol-unix
load-module module-native-protocol-unix
load-module module-null-source source_name=null
load-module module-pipe-source source_name=src format=s24le
load-module module-null-sink sink_name=dst
load-module module-loopback source=src sink=dst
$ ~/bin/pacat -r -d null --format=s24le > /tmp/music.input
$ ~/bin/pulseaudio -vvv
...
I: [null-sink] module-loopback.c: Adding 66663 usec of silence to queue
I: [null-sink] module-loopback.c: Adding 66663 usec of silence to queue
I: [null-sink] module-loopback.c: Adding 66663 usec of silence to queue
I: [null-sink] module-loopback.c: Adding 66663 usec of silence to queue
I: [null-sink] module-loopback.c: Adding 66663 usec of silence to queue
I: [null-sink] module-loopback.c: Dropping 17976 usec of audio from queue
E: [pipe-source] source-output.c: Assertion 'pa_frame_aligned(chunk->length, &o->source->sample_spec)' failed at pulsecore/source-output.c:738, function pa_source_output_push(). Aborting.
Aborted (core dumped)
$
I decided to do not open a bug because almost whole pipe-source should
be rewritten, and this is what I'm doing now.
--
Raman
