[added pulseaudio-discuss back to cc]
On Fri, 2015-11-06 at 11:29 +0100, Georg Chini wrote:
> On 06.11.2015 10:15, Tanu Kaskinen wrote:
> > On Wed, 2015-02-25 at 19:43 +0100, Georg Chini wrote:
> > > Now takes the difference of the timestamps when source and sink latency
> > > were captured into account.
> > >
> > > For now, for logging purposes only.
> > >
> > > Even though the timestamp difference is correctly taken into account for
> > > the case when the nominal and actual rates match, care is taken to
> > > minimize this difference. For this purpose, the order of snapshotting
> > > the sink and the source is swapped, because the sink snapshot usually
> > > takes longer then the source shapshot.
> > >
> > > Note: for both webcam -> HDA and bluetooth a2dp -> HDA scenarios with
> > > 200 ms latency, the old and the new method give results that agree within
> > > 0.5 ms. So, while theoretically an improvement, on my hardware, the patch
> > > is effectively a no-op for such latencies.
> > > There are however situations where the delay between sink and source
> > > snapshot becomes significant.
> > > ---
> > > Â src/modules/module-loopback.c | 36 +++++++++++++++++++++++++-----------
> > > Â 1 file changed, 25 insertions(+), 11 deletions(-)
> > Sorry for the long review delay. I hope you still remember enough of
> > your thinking process so that you can answer my questions.
>
> Thanks for starting the review.
>
> >
> > > diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
> > > index 1b34657..3532728 100644
> > > --- a/src/modules/module-loopback.c
> > > +++ b/src/modules/module-loopback.c
> > > @@ -173,38 +173,52 @@ static void teardown(struct userdata *u) {
> > > Â static void adjust_rates(struct userdata *u) {
> > > Â Â Â Â Â Â size_t buffer, fs;
> > > Â Â Â Â Â Â uint32_t old_rate, base_rate, new_rate;
> > > -Â Â Â Â pa_usec_t buffer_latency;
> > > +Â Â Â Â pa_usec_t final_latency, current_buffer_latency, current_latency, corrected_latency;
> > > +Â Â Â Â int32_t latency_difference;
> > > +Â Â Â Â pa_usec_t snapshot_delay;
> > > Â
> > > Â Â Â Â Â Â pa_assert(u);
> > > Â Â Â Â Â Â pa_assert_ctl_context();
> > > Â
> > > -Â Â Â Â pa_asyncmsgq_send(u->source_output->source->asyncmsgq, PA_MSGOBJECT(u->source_output), SOURCE_OUTPUT_MESSAGE_LATENCY_SNAPSHOT, NULL, 0, NULL);
> > > Â Â Â Â Â Â pa_asyncmsgq_send(u->sink_input->sink->asyncmsgq, PA_MSGOBJECT(u->sink_input), SINK_INPUT_MESSAGE_LATENCY_SNAPSHOT, NULL, 0, NULL);
> > > +Â Â Â Â pa_asyncmsgq_send(u->source_output->source->asyncmsgq, PA_MSGOBJECT(u->source_output), SOURCE_OUTPUT_MESSAGE_LATENCY_SNAPSHOT, NULL, 0, NULL);
> > > Â
> > > -Â Â Â Â buffer =
> > > -Â Â Â Â Â Â Â Â u->latency_snapshot.sink_input_buffer +
> > > -Â Â Â Â Â Â Â Â u->latency_snapshot.source_output_buffer;
> > > +Â Â Â Â /* Rates and latencies*/
> > > +Â Â Â Â old_rate = u->sink_input->sample_spec.rate;
> > > +Â Â Â Â base_rate = u->source_output->sample_spec.rate;
> > > Â
> > > +Â Â Â Â buffer = u->latency_snapshot.sink_input_buffer + u->latency_snapshot.source_output_buffer;
> > > Â Â Â Â Â Â if (u->latency_snapshot.recv_counter <= u->latency_snapshot.send_counter)
> > > Â Â Â Â Â Â Â Â Â Â buffer += (size_t) (u->latency_snapshot.send_counter - u->latency_snapshot.recv_counter);
> > > Â Â Â Â Â Â else
> > > Â Â Â Â Â Â Â Â Â Â buffer = PA_CLIP_SUB(buffer, (size_t) (u->latency_snapshot.recv_counter - u->latency_snapshot.send_counter));
> > > Â
> > > -Â Â Â Â buffer_latency = pa_bytes_to_usec(buffer, &u->sink_input->sample_spec);
> > > +Â Â Â Â current_buffer_latency = pa_bytes_to_usec(buffer, &u->sink_input->sample_spec);
> > > +Â Â Â Â snapshot_delay = u->latency_snapshot.source_timestamp - u->latency_snapshot.sink_timestamp;
> > > +Â Â Â Â current_latency = u->latency_snapshot.sink_latency + current_buffer_latency + base_rate * u->latency_snapshot.source_latency / old_rate - snapshot_delay;
> > > Â
> > > -Â Â Â Â pa_log_debug("Loopback overall latency is %0.2f ms + %0.2f ms + %0.2f ms = %0.2f ms",
> > > +Â Â Â Â final_latency = u->latency;
> > > +
> > > +Â Â Â Â /* Latency and latency difference at base rate */
> > > +Â Â Â Â corrected_latency = u->latency_snapshot.source_latency + (u->latency_snapshot.sink_latency + current_buffer_latency) * old_rate / base_rate - snapshot_delay;
> > I think there should be a comment explaining why this "correct latency"
> > is more correct than the incorrect latency... and what is the incorrect
> > latency? Is current_latency it?
>
> There are two latency values: The one is the real, current latency at the
> rate which is in use at that point in time. This is current_latency and this
> is the "correct" value at that time.
> The other is the latency you would get if you switched the rate back to
> the base rate. This is the corrected_latency. The name does not imply
> that it is more correct than the other latency, just that a rate correction
> was applied. And that is what the comment says (corrected latency
> = latency at base rate). Do you have some suggestion how to make it
> more clear?
Maybe rename the variable to latency_at_base_rate?
> The value is important because the goal of the latency regulation is not
> only to achieve the right latency but also to have that latency at the base
> rate.
The way you put it makes it sound like there are two goals: 1) achieve
the target latency at the real rate and 2) achieve the target latency
at the base rate. Is that really so? The two goals seem contradictory
(you can't meet both goals at the same time, unless the base rate and
the real rate are the same). I also don't understand why the latency at
the base rate is interesting, but maybe I'll understand once I'll get
to the regulator code...
> >
> > > +Â Â Â Â latency_difference = (int32_t)(corrected_latency - final_latency);
> > > +
> > > +Â Â Â Â pa_log_debug("Loopback overall latency is %0.2f ms + %0.2f ms + %0.2f ms = %0.2f ms (at the base rate: %0.2f ms, old estimate: %0.2f ms)",
> > The log message could be made easier to understand. I propose the
> > following format:
> >
> > Loopback latency:
> > Â Â Â Â Â source: %0.2f ms
> > Â Â Â Â Â buffers: %0.2f ms
> > Â Â Â Â Â sink: %0.2f ms
> > Â Â Â Â Â sum: %0.2f ms
> > Â Â Â Â Â sum (adjusted for measurement delay): %0.2f ms
> > Â Â Â Â Â sum (at the base rate): %0.2f ms
> > Â Â Â Â Â sum (old estimate): %0.2f ms
> >
> > Just one sum value might be enough, though. Why is the latency
> > calculated separately at the base rate? In theory the sum should be
> > independent of which rate domain is chosen, right?
>
> No, it is not. Imagine you have n samples between input and output.
> Then the difference between the two sums would be
> t = n / current_rate - n / base_rate.
Thanks, now I understand.
> I'll change the format to what you propose.
>
> >
> > Does the old estimate mean the estimate that the code did before your
> > patches? If so, that seems like something that you only use during
> > development of these patches, and shouldn't be included in the final
> > code.
>
> Yes, I'll change it accordingly.
>
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (double) u->latency_snapshot.sink_latency / PA_USEC_PER_MSEC,
> > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (double) buffer_latency / PA_USEC_PER_MSEC,
> > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (double) current_buffer_latency / PA_USEC_PER_MSEC,
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (double) u->latency_snapshot.source_latency / PA_USEC_PER_MSEC,
> > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ((double) u->latency_snapshot.sink_latency + buffer_latency + u->latency_snapshot.source_latency) / PA_USEC_PER_MSEC);
> > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (double) current_latency / PA_USEC_PER_MSEC,
> > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (double) corrected_latency / PA_USEC_PER_MSEC,
> > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ((double) u->latency_snapshot.sink_latency + current_buffer_latency + u->latency_snapshot.source_latency) / PA_USEC_PER_MSEC);
> > > +Â Â Â Â pa_log_debug("Latency difference: %0.2f ms, rate difference: %i Hz",
> > This message could be improved too. If the log reader is not familiar
> > with the code, it's not obvious what differences these are.
>
> It was clear to those who used it so far. There is only one parameter
> which is
> changed - the rate - and one parameter that is adjusted - the latency.
> I can use "deviation from base rate" and "deviation from requested latency"
> instead if you think this is more clear.
Yes, I think that's more clear.
--
Tanu
