On Sat, 2019-06-15 at 11:31 +0200, Georg Chini wrote: > On 15.06.19 10:01, Tanu Kaskinen wrote: > > On Tue, 2019-06-11 at 22:08 +0200, Georg Chini wrote: > > > Hi Tanu, > > > > > > the first diagram should look like this: > > > > > > DIAGRAM FOR HARDWARE SINK 1 BEFORE STARTING THE MOVE > > > > > > +---------------------------------------------------------------------------------+----- > > > > client stream memblockq | > > > +---------------------------------------------------------------------------------+----- > > > ^ read index > > > > > > +------------------------------------------------------------+--------------------+----- > > > > client history_queue | queue length | > > > +------------------------------------------------------------+--------------------+----- > > > ^ read index ^write index > > > > > > > > > +------------------------------+ > > > | data in the client resampler | > > > +------------------------------+ > > > > > > +--------------------------------------------------+ > > > > client render_memblockq | queue length | > > > +--------------------------------------------------+ > > > ^ read index ^ write index > > > > > > ----------------------------+ > > > | dma buffer | > > > ----------------------------+ > > > ^ hardware playback position, > > > can't rewind beyond this point > > > > > > This is why I do not need to take the render memblockq length into account when > > > rewinding the history queue. (When rewinding during a volume change, the history > > > queue is also rewound by the same amount as the render memblockq plus the data in > > > the resampler. Then the resampler bit is fed back into the resampler.) > > > > > > The third diagram is correct again, before finishing the move, the read pointers are > > > out of sync and will be re-synchronized when pa_sink_input_drop() is called the next > > > time during FINISH_MOVE. > > I don't see why history_queue length should be in sync with the > > render_memblockq length. I find your scheme harder to understand, > > because the usual rule of the write position of a downstream buffer > > matching the read position of the next buffer upstream doesn't hold. In > > your scheme the read index of history_queue doesn't point to the > > location from which the resampler reads data. Could you change this bit > > in your code? > > > I do not read from the history queue during normal operation, the input > data for the resampler comes from the client. I just push the data that > the client provides into the history. Ok, so the history_queue isn't part of the audio flow, it's just a separate buffer with its own special rules. To make it part of the audio flow, you could read a chunk from the client, push it to the history_queue, then immediately read the chunk back from the history_queue and push it to the resampler. > It is much simpler to keep the indices in sync than to have to care about > the read index difference between the render memblockq and the history > queue. What is there to care about? The read index difference is the render_memblockq length plus the resampler delay. Those will keep up to date by themselves, no manual intervention needed. > This basically means that I can do the same operations on the history > queue that I do on the render memblockq. > The history queue is only read in two situations: > > 1) During a volume change. In this case I can now simply rewind the history > queue by the same amount that I rewind the render memblockq plus the bit > that I have to feed into the resampler. Otherwise you'd simply rewind the history queue by the new render_memblockq length (after rewinding it first) plus the resampler delay. Doesn't seem any more complicated to me. > 2) During a move. Here again it helps that the indices are in sync because I > do not need to save the render memblockq length during the START_MOVE > message. I can simply ignore the bit in the render queue that was not yet > read because it will be taken into account automatically. So instead of having a variable "old_render_memblockq_length" you use the history_memblockq length as a substitute, which makes it necessary to fiddle with the read index to keep it sync with the render_memblockq. I don't see how that promotes clarity - the variable name is much more descriptive than pa_memblockq_get_length(i->history_queue), because latter doesn't have any obvious connection to render_memblockq (hopefully you at least have a comment explaining that the history_queue length is the same as the old render_memblockq length). > Additionally, I have to do something with the read index of the history > queue anyway, because it is never read during normal operation. So if I > do not drop the data somewhere, the length will keep growing. > > So all in all I think it makes the code much better readable and > understandable > if the history queue is kept in sync with the render queue. It is a > history queue > and as such should reflect what happens to the render queue. I can still > change > this if you want, but for me it does not make sense to complicate the code > unnecessary. I have hard time believing the "much better readable and understandable" code argument. I think not having the history_queue part of the normal audio flow makes the system substantially harder to reason about (and to visualize). -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
