Created attachment 262902 [details] [review]
tests: add audiosink test for late buffers

Patch that adds check test for this issue

I think this is voluntary, that the choice is to break sync a bit instead of inserting an audible break in the sound. But maybe we should insert a discont above a certain threshold?

Created attachment 263170 [details] [review]
tests: add audiosink test for late buffers

Improved version for tests that also checks for partially late buffers

Created attachment 263171 [details] [review]
audiobasesink: resync on resync not discont

As discussed on IRC, it seems that DISCONT doesn't relate to timestamps
but to the content, while what it was meant in this piece of code
was the RESYNC flag

Created attachment 263172 [details] [review]
audiobasesink: fix dropping of late samples

This isn't a proper fix but I'm having a hard time figuring out this
part of the code, it seems that there is something missing or wrong
on the next_sample, sample_offset and align math here.

If I only switch the inequation as this patch does everything works
for my use case

Created attachment 264563 [details] [review]
audiobasesink: properly detect samples behind current playback position

Fix the expected vs received sample offsets comparison to properly
detect samples that are behind current playback position.

This also requires using _int_scale_round variant to avoid
ignoring samples that would be misaligned by 1 sample.

Change makes sense to me. just wondering whether chunk #1
should be committed separately. Whatever the case it's indeed
quite a finding, congrats.

I think the original code was correct. The idea is that when we have calculated that the next sample should be played at ->next_sample and the new sample at sample_offset, we have:

headroom: the diff between ringbuffer read pointer and our sample_offset. This should be positive if we are writing before the ringbuffer readpointer (and want to have a chance to hear the sample still)

sample_diff: absolute diff between ->next_sample and sample_offset. Or the error between what we expected to receive and what we received.

align = sink->next_sample - sample_offset; ==0 means perfect, < 0 means the sample is to be played later than expected and thus alignment would play the sample earlier, > 0 the sample is to be played sooner than expected and thus alignment would delay playback.

The we have:

  /* don't align if it means writing behind the read-segment */
  if (sample_diff > headroom && align < 0)
    allow_align = FALSE;

If the sample error is bigger than the diff between read and write pointer *and* alignment would make us play the sample earlier (earlier than the headroom, thus before the read pointer) then we don't align.

This makes sense because aligning to before the read pointer would 1) play the sample wrongly (it would be aligned) 2) it would simply drop the sample because the ringbuffer has already read past the sample. 

What exactly is it that you think is wrong?

Also, dropping of late samples (samples before the read pointer) is done in 

gst-libs/gst/audio/gstaudioringbuffer.c:1514

/* segment too far ahead, writer too slow, we need to drop, hopefully UNLIKELY */
if (G_UNLIKELY (diff < 0)) {
  /* we need to drop one segment at a time, pretend we wrote a segment. */

This is done when calculating where in the ringbuffer the sample should be inserted, the diff is compared with the current read pointer and the sample is dropped when it is before the read pointer.

(In reply to comment #8)
I don't understand this.

> align = sink->next_sample - sample_offset; ==0 means perfect, < 0 means the
> sample is to be played later than expected and thus alignment would play the
> sample earlier, > 0 the sample is to be played sooner than expected and thus
> alignment would delay playback.
> 
> The we have:
> 
>   /* don't align if it means writing behind the read-segment */
>   if (sample_diff > headroom && align < 0)
>     allow_align = FALSE;

If the align being negative means that the sample is to be played later than expected, how does comparing "align < 0" make sense here to check if it is before the read-segment?

-----|--------------------------|------------->
    current                    expected
    play position              sample offset

The headroom is the difference between those 2 positions, if the sample_diff
(difference between the sample offset and the expected sample offset) is 
greater than the headroom it means that we could end up writing it before the
current play position. We don't want this to happen. How does comparing with 
'align < 0" works here if it means that sample should be played later?

> 
> If the sample error is bigger than the diff between read and write pointer
> *and* alignment would make us play the sample earlier (earlier than the
> headroom, thus before the read pointer) then we don't align.
>

(In reply to comment #8)
> [..]
> align = sink->next_sample - sample_offset; ==0 means perfect, < 0 means the
> sample is to be played later than expected and thus alignment would play the
> sample earlier, > 0 the sample is to be played sooner than expected and thus
> alignment would delay playback.
> [..]

Wim: Isn't this just backwards ^ ? Srsly confused now, will take another
look at the code.

(In reply to comment #11)
> (In reply to comment #8)
> > [..]
> > align = sink->next_sample - sample_offset; ==0 means perfect, < 0 means the
> > sample is to be played later than expected and thus alignment would play the
> > sample earlier, > 0 the sample is to be played sooner than expected and thus
> > alignment would delay playback.
> > [..]
> 
> Wim: Isn't this just backwards ^ ? Srsly confused now, will take another
> look at the code.

The confusion is from the fact that alignment makes the sample play at the wrong time, the timestamp converted to sample_offset is ignored and the sample is played at the expected next position, aligned to the previous sample.

You don't want to align if that next_sample is behind the read pointer, you prefer to play the sample at its intended timestamp.

Is there anything still needed here? From the discussion on IRC it seems that this code is doing what it is supposed to do and there is no problem here?

Yes, seems that it works. Let's close this as invalid, then.