[pulseaudio-discuss] Resampler rewinding

[Tanu Kaskinen]

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