[pulseaudio-discuss] [RFC PATCH] First draft implementation of better drain reporting

[Tanu Kaskinen]

On Mon, 2013-03-04 at 12:33 +0100, David Henningsson wrote:
> On 03/02/2013 10:41 PM, Tanu Kaskinen wrote:
> > On Fri, 2013-03-01 at 16:05 +0100, David Henningsson wrote:
> >> @@ -635,12 +641,14 @@ static int mmap_write(struct userdata *u, pa_usec_t *sleep_usec, pa_bool_t polle
> >>
> >>               p = (uint8_t*) areas[0].addr + (offset * u->frame_size);
> >>
> >> -            chunk.memblock = pa_memblock_new_fixed(u->core->mempool, p, frames * u->frame_size, TRUE);
> >> +            written = frames * u->frame_size;
> >> +            chunk.memblock = pa_memblock_new_fixed(u->core->mempool, p, written, TRUE);
> >>               chunk.length = pa_memblock_get_length(chunk.memblock);
> >>               chunk.index = 0;
> >>
> >>               pa_sink_render_into_full(u->sink, &chunk);
> >>               pa_memblock_unref_fixed(chunk.memblock);
> >> +            u->since_fill = 0;
> >
> > Shouldn't this be done only after a successful snd_pcm_mmap_commit()
> > call?
> 
> I'm not sure.
> 
> pa_sink_render_into_full is what's causing new data to be mixed, so one 
> could see it as since filling up render_memblockq, rather than filling 
> up the DMA area.

I was seeing it as since filling up the DMA area, because it's
calculated based on the DMA area fill level. But I think the correct
interpretation is indeed to look at it from the sink input point of
view, due to the context where it's used (in pa_sink_find_underrun()).

> >> +    input_underrun = pa_sink_find_underrun(u->sink, u->since_fill, left_to_play);
> >
> > I believe left_to_play is not the correct variable to use here. It is
> > based on snd_pcm_avail(), but we're interested in the total latency, so
> > snd_pcm_delay() should be used.
> >
> > Hmm, on the other hand, the new device underrun notifications should be
> > sent as soon as the input underrun "leaves" the alsa ring buffer. So,
> > the drain and underrun notifications should happen at different times.
> 
> I see your point, but having them both notify at the different times is 
> an additional complexity that maybe we shouldn't deal with right now.
> 
> To ask a counter-question; is there a use case where it hurts that we 
> haven't been including snd_pcm_delay when reporting back our drain? I 
> mean, we won't close the sink for an additional five seconds, so there 
> shouldn't be any risk that the end of the sound gets chopped.
> And for the people who want to play back several wave files in a row, 
> the proposed handling should be slightly better, right?

Right. I was thinking about this from the point of view that drain()
should return when all audio has hit the speakers, but that doesn't
appear to be a useful viewpoint in practice. As you point out, that
interpretation of drain() causes delays when creating a new stream after
the previous has drained.

> >> @@ -677,6 +693,11 @@ static int mmap_write(struct userdata *u, pa_usec_t *sleep_usec, pa_bool_t polle
> >>               *sleep_usec -= process_usec;
> >>           else
> >>               *sleep_usec = 0;
> >> +        if (underrun_sleep > 0) {
> >> +            underrun_sleep += process_usec; /* Add some margin. I haven't figured out why we need this :-( */
> >
> > I think the margin should be a separate variable/constant. I don't think
> > there's any connection between process_usec and the margin that we need
> > here.
> 
> There is also the rewind_safeguard stuff. I haven't figured if or when 
> that parameter should be involved...

I don't think that's related either. AFAIK, rewind_safeguard exists,
because it may be unsafe to write too close to the read pointer. That's
a separate problem from the "woke up sooner than intended" problem.

> >> +static pa_bool_t sink_input_verify_underrun_cb(pa_sink_input *i);
> >
> > s/pa_bool_t/bool/, s/TRUE/true/ and s/FALSE/false/.
> 
> A question; I'm not sure when we decided to ditch pa_bool_t, but anyway. 
> It feels a bit weird to mix both in the same file. Wouldn't it make more 
> sense to convert an entire file at once, instead of mixing?

Well, my policy is to use stdbool in all new code, and convert old code
when it's touched. It doesn't bother me much that this causes
inconsistency within files. If it bothers you, it's totally OK for me if
you write patches that convert whole files (or more) in one go, as long
such patches are separate from patches that change behavior.

As for when we decided to ditch pa_bool_t: here's a thread where I made
the proposal, and it didn't face opposition:
http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/13647

Some further discussion (such as asking from Lennart why we have
pa_bool_t in the first place) took place in IRC.

> >>   /* Called from thread context */
> >> +pa_bool_t pa_sink_input_verify_real_underrun(pa_sink_input *i, size_t nbytes /* in the sink's sample spec */) {
> >
> > I'd rename nbytes to underrun_for. A comment can be added explaining why
> > it's not necessarily equal to i->thread_info.underrun_for.
> 
> nbytes is used to specify sink bytes in the surroundings, so I'm 
> claiming the consistency argument for this one.

I think the purpose of the variable is very unobvious. If you add a
comment explaining that the variable is based on
i->thread_info.underrun_for, I'm OK with "nbytes" too.

> >
> >> +    pa_sink_input_assert_ref(i);
> >> +    pa_sink_input_assert_io_context(i);
> >> +
> >> +    if (nbytes < pa_memblockq_get_maxrewind(i->thread_info.render_memblockq))
> >> +        return FALSE;
> >> +    if (pa_memblockq_is_readable(i->thread_info.render_memblockq))
> >> +        return FALSE;
> >> +
> >> +    if (!i->verify_underrun)
> >> +        return FALSE;
> >> +    if (i->verify_underrun(i)) {
> >> +        /* All valid data has been played back, so we can empty this queue. */
> >> +        pa_memblockq_silence(i->thread_info.render_memblockq);
> >
> > Why is pa_memblockq_silence() needed?
> 
> It is a precondition for pa_sink_input_safe_to_remove. I'm not sure it's 
> needed now, as we already have sent the drain ack at this point. See it 
> as an optimisation.

OK, I'm not 100% sure either.

> >> +        size_t uf = pa_sink_input_get_underrun_for_sink(i);
> >
> > As a minor optimization and also for better readability, I think it
> > would be better to maintain a separate for_underrun_sink variable in
> > i->thread_info instead of having pa_sink_input_get_underrun_for_sink().
> 
> If so, I think there should still be some function, perhaps an inline 
> function or so, because I don't like that a pa_sink_* function would 
> directly peek i's variables.

In my opinion reading variables is fine across classes, unless the
variable is clearly such that nobody outside the class implementor
should be interested in it (and if you have pa_foo_get_bar(), then
pa_foo.bar is certainly not such variable). For writing variables, I
agree that pa_foo_set_bar() should always be used.

The benefit from accessing the variable directly is that the reader sees
that there's no magic happening. pa_foo_get_bar() may or may not have
some magic.

The downside of not having a policy of always using pa_foo_get_bar() is
that in the rare cases where such accessor function should be used, it
can be forgotten when writing new code.

The current code base certainly doesn't have a policy of always using an
accessor function for reading variables. Do you propose that we adopt
such policy?

> >> +        if (uf == 0)
> >> +            continue;
> >> +        uf += since_fill;
> >> +        if (pa_sink_input_verify_real_underrun(i, uf))
> >
> > This notifies an input about a real underrun. I think that's a bit
> > surprising side effect for pa_sink_find_underrun(), so I think it would
> > be clearer to have a separate function for doing those notifications,
> > called before pa_sink_find_underrun().
> 
> I did this as a minor optimisation, in order not to traverse the hashmap 
> one extra time.
> 
> Perhaps pa_sink_process_input_underruns() for a function that does both?

Sounds good.

-- 
Tanu