[pulseaudio-discuss] [PATCH v6 09/25] loopback: Refactor latency initialization

[Tanu Kaskinen]

On Thu, 2016-07-14 at 11:57 +0200, Georg Chini wrote:
> On 14.07.2016 00:02, Tanu Kaskinen wrote:
> > On Sun, 2016-06-05 at 21:05 +0200, Georg Chini wrote:
> > > +    /* Latency boundaries and current values */
> > > +    pa_usec_t min_source_latency;
> > > +    pa_usec_t max_source_latency;
> > > +    int64_t source_offset;
> > > +    pa_usec_t min_sink_latency;
> > > +    pa_usec_t max_sink_latency;
> > > +    int64_t sink_offset;
> > 
> > This patch doesn't use source_offset or sink_offset for anything, so it
> > doesn't look like a good idea to add them in this patch.
> 
> The commit message says that I add some variables for later use.
> I thought it was more convenient here because this way all the
> boundary variables are introduced in one patch which makes the
> other patches better readable. I can however move the variables
> if you prefer.

I would prefer that, thanks.

> > "source_latency_offset" and "sink_latency_offset" would be clearer
> > names.
> > 
> > Also, the user may reconfigure the latency offsets at any time, so if
> > you're going to use the offsets for something, you'll need to monitor
> > them for changes.
> 
> How can I track those changes? I don't see any event I could use.
> The offsets are later used to determine the minimum possible
> target latency.

I suppose you need to add new hooks:
PA_CORE_HOOK_SINK/SOURCE_LATENCY_OFFSET_CHANGED or something like that.
The matter is complicated by the fact that it will be possible to set
the offset separately for the port and the sink (currently setting only
the port latency offset is supported, but there's a patch set posted
that adds configurable sink latency offset too). What you need is hooks
for getting notifications of the total (sum of sink and port) offset
changes, but there will also be a hook for monitoring just the sink
latency offset part. Maybe the hooks for the total offset could be
named PA_CORE_HOOK_SINK/SOURCE_EFFECTIVE_LATENCY_OFFSET_CHANGED.

> > > +/* Called from all contexts
> > 
> > Well, that's not a safe thing to do.
> 
> I agree that it is potentially unsafe. Unfortunately the procedure must
> be called from main and IO context. I believe it is safe in this special 
> case
> because I prevent adding silence to the queue when calling from IO context.
> Is this OK for you?

No, it's still not safe. At least the pa_memblockq_get_length() and
pa_memblockq_drop() calls are unsafe from other threads than the sink
IO thread (unless you know that the sink IO thread isn't running -
there's at least one instance where memblockq_adjust() is called when
the sink input isn't connected to any sink).

You can send a synchronous message from the main thread to the sink IO
thread to do the necessary memblockq adjustments.

> > > +    u->configured_source_latency = pa_source_output_set_requested_latency(u->source_output, latency);
> > > +    if (u->configured_source_latency != requested_latency)
> > > +        pa_log_warn("Cannot set requested source latency of %0.2f ms, adjusting to %0.2f ms", (double)requested_latency / PA_USEC_PER_MSEC, (double)u->configured_source_latency / PA_USEC_PER_MSEC);
> > 
> > If the actual configured latency of either sink or source is higher
> > than expected, then it may be that the configured source latency plus
> > the configured sink latency is higher than the configured overall
> > latency. In this case the target overall latency should be pushed up,
> > so that we don't try to target an impossible latency.
> 
> Restricting the target latency to (hopefully) sane values is done in 
> another patch.

Ok.

> > >           case SINK_INPUT_MESSAGE_REWIND:
> > >   
> > >               pa_sink_input_assert_io_context(u->sink_input);
> > >   
> > > -            if (PA_SINK_IS_OPENED(u->sink_input->sink->thread_info.state))
> > > +            if (PA_SINK_IS_OPENED(u->sink_input->sink->thread_info.state) && u->pop_called)
> > >                   pa_memblockq_seek(u->memblockq, -offset, PA_SEEK_RELATIVE, true);
> > >               else
> > > -                pa_memblockq_flush_write(u->memblockq, true);
> > > +                /* If the sink is not yet playing, adjust the buffer to contain
> > > +                 * somewhat more than the configured latency, a rewind does not
> > > +                 * make sense in that case. Use a quarter of the current sink
> > > +                 * latency as safety margin */
> > > +                memblockq_adjust(u, (int32_t)(-u->configured_sink_latency / 4), false);
> > 
> > Why wouldn't the rewind make sense? (Well, source rewinding in general
> > is something I'm not sure makes a whole lot sense, but let's ignore
> > that.) I think the rewinding here means that the source told that the
> > last bit it sent out should be discarded. Since you're queuing all sent
> > data in the POST message handler even when the sink isn't running, I
> > think pa_memblockq_seek() would make sense here to get rid of the
> > recently received audio.
> > 
> > Since pa_memblockq_seek() will make the memblockq shorter, I don't
> > think memblockq_adjust() makes sense. I don't know what your original
> > purpose of calling memblockq_adjust() here was.
> 
> Do you think just using memblockq_seek() is sufficient?

Yes.

> The current
> code flushes the memblockq when a rewind is received.

Indeed, but the current code doesn't try to keep the memblockq filled
with valid data from the source while the sink isn't running. Without
valid data buffered, rewinding does no good.

> > > +    /* Fill the memblockq with silence to avoid underruns at startup.
> > > +     * For dynamic latency devices it is enough to use the configured
> > > +     * loopback latency, else add some safety margin. A quarter of the
> > > +     * configured sink latency should be sufficient */
> > > +    if(u->sink_input->sink->flags & PA_SINK_DYNAMIC_LATENCY)
> > > +        memblockq_adjust(u, 0, true);
> > > +    else
> > > +        memblockq_adjust(u, (int32_t)(-u->configured_sink_latency / 4), true);
> > 
> > Here again I don't understand why any safety margin beyond the target
> > latency would be needed. Is it to handle the case where the source is
> > initially stopped and it takes a while before it pushes out the first
> > chunk of audio? I think it would be better to handle this similarly to
> > how you wait until pop() has been called for the first time. You don't
> > know how long startup delay the source is going to have.
> 
> As you suspect, the safety margin is for the case that the source takes
> longer to start up. With the current code underruns cannot be avoided
> if the source takes too long, because source and sink are started at the
> same time. Once the sink is running, it will consume data.
> Do you think I should wait for the source to deliver some data before I
> connect the sink to the sink input? This would mean a somewhat longer
> delay before the loopback produces any output because I will have to
> wait for the sink again.

I was thinking of modifying the pop() callback so that when it reads
something from the memblockq, it would also push an equivalent amount
of silence to the queue if the source hasn't yet produced any data.

-- 
Tanu