[pulseaudio-discuss] Resampler rewinding

[Georg Chini]

On 16.06.19 10:39, Tanu Kaskinen wrote:
> 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.

Yes, I started like that but it is much simpler to keep it out of the
flow. Data is pushed into the queue when it is received and
dropped in pa_sink_input_drop() to match the length of the
render memblockq.

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

Yes, but as said below, you have to add the render memblockq
length in multiple places.


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

It is only a bit more complicated. But you will have to do
the same when changing volume or doing anything that
involves feeding data from the history queue into the
resampler or the memblockq.


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

"Fiddeling with the read index" must be done each time the
queue is used for something if you don't keep it in sync.
So I prefer doing it in one place.


> 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).
>
The point is, that in all places where you use the history queue,
you will have to take the render memblockq length into account.
So I am doing it in one place in pa_sink_input_drop() instead of
everywhere where the history queue is used. I think the code
is pretty clear as it is now - each operation on the render queue
is mirrored exactly in the history queue. This would not be
possible if it is not kept in sync. And the code to keep it in sync
consists of a few lines and has no risk that the two queues
get out of sync because of rounding errors when converting
between the sample specs.
The above is another argument for keeping it as it is - if there
are multiple rewinds and you don't do anything to keep the
queues in sync, they may start to differ because rewinding
x samples on one queue means rewinding y.z samples on
the other queue. I have seen this type of problem when
I started with the patch set.