[pulseaudio-discuss] [PATCH v9] pipe-source: [DRAFT] implement autosuspend behavior

[Raman Shishniou]

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 / 1000000000 ~ 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 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.
> 
> No, you pick up reading exactly where you stop. There is no other reader.
> So you can do the same thing you would do when you post the data, which
> is discard full frames starting at the head of the buffer and then move the
> remaining fragment to the head.
> If the writer disconnects during suspend, we can assume that it delivered
> full frames, so in that case there should be no fragment left.
> 
> You are however right that there is a situation where it is possible that we
> loose frame boundaries. Whenever the pipe gets filled completely, the
> writer might do the wrong thing or it might disconnect, leaving some
> fragment in the pipe.
> 
> So, yes, I agree to continue polling during suspend. Or could we make that
> behavior optional? I'm not sure how much more work it is to implement both
> alternatives.
> 

Ok, I'll try to do something like this, but it should be another patch on top
to avoid too many changes in one.

>>
>>>> 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.
>>
> They surely will because they are fixing a release-blocker bug.

--
Raman