GNOME Bugzilla – Bug 725078
audiobasesink: clip start samples to match clipped timestamp from skew algorithm
Last modified: 2015-02-22 13:12:09 UTC
Created attachment 270159 [details] [review] Clip start samples Clock slaving can clip start time to zero, giving us a shorted duration than we originally got. To keep in sync, we must then discard the samples falling before that zero timestamp. The condition is not easy to get, so I'm not 100% sure it fixes the issue I was having (constant PA underflows causing heavy distortion).
Pushed to master and 1.2: commit 169166d0a2ef735576870f41a5eb9c38da337451 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Feb 24 11:17:05 2014 -0500 audiobasesink: clip start samples to match clipped start time Clock slaving can clip start time to zero, giving us a shorted duration than we originally got. To keep in sync, we must then discard the samples falling before that zero timestamp. This possibly fixes random distortion caused by constant PA underflows which are never resynced.
> The condition is not easy to get, so I'm not 100% sure it fixes > the issue I was having (constant PA underflows causing heavy > distortion). If you don't know if this fixed anything, you probably shouldn't have picked it into 1.2. It's not really "obviously correct" IMHO. <wtay> __tim, , it looks like it would simply resample. I don't understand what this patch fixes exactly. Vincent, could you elaborate?
It's hard to test, but I'm reasonably sure it fixed my issue. It seems theoretically correct to me, what do you see as not obviously correct ? The patch fixes an issue when the skew algorithm gives timestamps for incoming samples that cross 0 (ie, the start timestamp is negative, and hte end timestamp is positive). The start timestamp will then get clipped to 0, giving an interval tha'ts too short for the audio data. When this happens, the patch clips out the samples that lie before the newly clipped time, if there are any.
To explain more, the 0 timestamp before clipping the start time would match some sample in the middle of the data. Clipping to 0 means playback would start at the first sample in the data, pushing the original 0 time sample to the future.
Does it make sense to you with the above explanation ? Once I found the issue, the fix seems logical to me, but maybe I'm missing something in how those timestamps are meant to be changed by the skew algorithm...
Makes sense to me, setting to NEEDINFO till I see if my explanation convinces Tim.
Sorry to ping again on this one, but the patch seems obviously correct to me, so I'll try to explain it with an example. Imagine you get a buffer with ts 3, duration 2, and 200 samples. That makes 100 samples per second, starting at 3 and ending at 5. Imagine now that the skew algorithm offsets the timestamps by -3.5: the start value goes from 3 to -0.5, end to 1.5. The start value will get clipped to 0, so the buffer will start at 0 and have duration 1.5. Those samples corresponding to the -0.5 - 0 interval now need to be dropped in order for the samples in positive time to play at the correct time. So dropping the first 50 samples gives us a buffer with the last 150 samples, starting at 0, and ending at 1.5. Does this convince you ?
Can I close this ? The patch has been in master for almost a year now, and I don't recall seeing bug reports related to it.
Thanks Vincent, I think it makes sense, yes. I agree that there's no point keeping this open.