On Sun, 2016-06-05 at 21:05 +0200, Georg Chini wrote:
> +/* It has been a matter of discussion how to correctly calculate the minimum
> + * latency that module-loopback can deliver with a given source and sink.
> + * The calculation has been placed in a separate function so that the definition
> + * can easily be changed */
> +static void update_minimum_latency(struct userdata *u) {
This function is called from many contexts, but that's not safe. The
mimimum_latency variable is used in adjust_rates() and
memblockq_adjust(). adjust_rates() is called from the main thread, so
when the IO threads notice that minimum_latency needs to be updated,
they should send a message to the main thread. memblockq_adjust() is
called from the sink IO thread, so minimum_latency needs to be saved
also in a separate variable that is only used from the sink IO thread.
Whenever the "main" minimum_latency variable changes, the main thread
should send a message to the sink IO thread to update the other
variable.
Note that IO thread -> main thread messages should be asynchronous
(pa_asyncmsgq_post) and main thread -> IO thread messages should be
synchronous (pa_asyncmsgq_send).
I'd be happy if you could somehow clearly divide the variables in
userdata based on from which context they're supposed to be accessed.
For example, pa_sink has field "thread_info", which is an embedded
struct that holds the sink's IO thread variables, and there are other
similar uses of a "thread_info" struct elsewhere. You could add
"sink_thread_info" and "source_thread_info" to the userdata struct.
> +
> +Â Â Â Â u->minimum_latency = u->min_sink_latency;
> +Â Â Â Â if (u->fixed_alsa_source)
> +Â Â Â Â Â Â Â Â u->minimum_latency += u->core->default_fragment_size_msec * PA_USEC_PER_MSEC;
> +Â Â Â Â else
> +Â Â Â Â Â Â Â Â u->minimum_latency += u->min_source_latency / 2;
I think there should be comments explaining why the minimum latency is
calculated as it is. The "fixed alsa source" case is calculated that
way, because we get a wakeup when one fragment is filled, and then we
empty the source buffer, so the source latency never grows much beyond
one fragment (assuming that the CPU doesn't cause a bottleneck), but I
don't remember why in the "not fixed alsa source" case you divide the
source latency by 2. And even if I understood all the reasons, they are
unobvious to people who haven't read through our previous discussion in
the recent past (I will eventually forget the discussion too).
> +/* Called from input thread context */
> +static void update_source_requested_latency_cb(pa_source_output *i) {
> +Â Â Â Â struct userdata *u;
> +Â Â Â Â pa_usec_t source_latency;
> +
> +Â Â Â Â pa_source_output_assert_ref(i);
> +Â Â Â Â pa_source_output_assert_io_context(i);
> +Â Â Â Â pa_assert_se(u = i->userdata);
> +
> +Â Â Â Â /* Source latency may have changed */
> +Â Â Â Â source_latency = pa_source_get_requested_latency_within_thread(u->source_output->source);
> +Â Â Â Â if (source_latency > u->configured_source_latency) {
> +Â Â Â Â Â Â Â Â /* The minimum latency has changed to a value larger than the configured latency */
> +Â Â Â Â Â Â Â Â pa_log_warn("Source latency increased to %0.2f ms", (double)source_latency / PA_USEC_PER_MSEC);
> +Â Â Â Â Â Â Â Â u->configured_source_latency = source_latency;
> +Â Â Â Â Â Â Â Â u->min_source_latency = source_latency;
Now you only ever increase the configured latency, but if the source at
some point lowers the minimum latency again, should we update the
latency configuration in module-loopback too? I think this can't
currently happen, and I'm ok with keeping this code as it is, but
decreasing the minimum latency might happen at some point, so preparing
for that would not be entirely pointless.
> @@ -1064,11 +1126,15 @@ int pa__init(pa_module *m) {
> Â Â Â Â Â u->source_output->may_move_to = source_output_may_move_to_cb;
> Â Â Â Â Â u->source_output->moving = source_output_moving_cb;
> Â Â Â Â Â u->source_output->suspend = source_output_suspend_cb;
> +Â Â Â Â u->source_output->update_source_requested_latency = update_source_requested_latency_cb;
> Â Â Â Â Â u->source_output->userdata = u;
> Â
> Â Â Â Â Â update_latency_boundaries(u, u->source_output->source, u->sink_input->sink);
> Â Â Â Â Â set_source_output_latency(u, u->source_output->source);
> Â
> +Â Â Â Â if (u->latency < u->minimum_latency)
> +Â Â Â Â Â Â Â pa_log_warn("Latency limited to %0.2f ms with current combination of source and sink", (double)u->minimum_latency / PA_USEC_PER_MSEC);
If you want to log this warning, I think the warning should also be
logged when moving to a new sink or source.Â
--Â
Tanu
