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. > > 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. > > 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).
