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.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
