GNOME Bugzilla – Bug 720692
audioringbuffer: track last position across release/acquire
Last modified: 2016-08-02 14:49:34 UTC
Created attachment 264494 [details] [review] audioringbuffer: track last position across release/acquire If releasing and reacquiring the ring buffer, such as can happen when doing gapless playback with playbin, the ring buffer would get its segone reset to 0, causing it to write as much data out as was written previously to get back to the same point. Keeping track of the last position fixes this. The position is tracked in samples, converted to time, and reconverted to samples with the new sample rate, in case it changes. This fixes playbin gapless playback with alsasink. I'm far from certain about this not breaking other stuff. It seems to work fine here so far. The breakage was introduced by https://bugzilla.gnome.org/show_bug.cgi?id=697723. Hopefully it doesn't reintroduce it...
Review of attachment 264494 [details] [review]: This looks generally good but ::: gst-libs/gst/audio/gstaudioringbuffer.c @@ +691,3 @@ + /* keep track of where were are in time, in case we get re-acquired */ + buf->x.x.sample_base = gst_util_uint64_scale_round (buf->x.x.sample_counter, + GST_SECOND, buf->spec.info.rate); This contains time for some time, and samples the remaining time. Please use two different variables to make this clear and also mark the values as invalid when they should not be used anymore @@ +1600,2 @@ done: + buf->x.x.sample_counter = *sample; You never use the value stored in sample_counter.
Thanks for reviewing, > This contains time for some time, and samples the remaining time. Please use > two different variables to make this clear and also mark the values as invalid > when they should not be used anymore OK, fixed > done: > + buf->x.x.sample_counter = *sample; > > You never use the value stored in sample_counter. I do, it is used in gst_audio_ring_buffer_release (to convert to time).
Created attachment 265212 [details] [review] track last position across release/acquire
> > This contains time for some time, and samples the remaining time. Please use > > two different variables to make this clear and also mark the values as invalid > > when they should not be used anymore > > OK, fixed It does eat 8 more bytes off the reserved space though. 24 bytes altogether. I'm just thinking I could make the time and sample bases a union, since their scope do not overlap. This would make them still use the same storage, but be "logically" different for clarity. Would you be OK with that, or do you think it's not worth it ?
(In reply to comment #2) > Thanks for reviewing, > > > This contains time for some time, and samples the remaining time. Please use > > two different variables to make this clear and also mark the values as invalid > > when they should not be used anymore > > OK, fixed You still store time or samples in the same variable. Please don't and use two variables for that :) After acquire() it is samples, after release() it is time.
(In reply to comment #4) > > > This contains time for some time, and samples the remaining time. Please use > > > two different variables to make this clear and also mark the values as invalid > > > when they should not be used anymore > > > > OK, fixed > > It does eat 8 more bytes off the reserved space though. 24 bytes altogether. > I'm just thinking I could make the time and sample bases a union, since their > scope do not overlap. This would make them still use the same storage, but be > "logically" different for clarity. Would you be OK with that, or do you think > it's not worth it ? Just use the instance private data. I don't think this needs to be public?
Hah, I uploaded the same old patch, sorry :) Yes, good point about private data, I will add one and use that.
I'm not sure the ringbuffer is the right place to fix this. It seems reasonable that the ringbuffer resets to reading from 0 after a release/acquire and that it's equally valid to say that the problem is the consumer (baseaudiosink / baseaudiosrc) doesn't adjust their write/read pointers after a caps change.
Created attachment 265213 [details] [review] track last position The updated patch, this time. With a new private data struct to store the new fields. As for ringbuffer/audiosink location, this is how this worked before before another fix change this. I'm not familiar enough with the code to really have an opinion on where it's best fixed, though the "keeping track" patch doesn't feel as clean as I'd like it to be. What do you think, slomo ? :)
I think Jan currently knows more about the details in that code than I do, it's too long since I last worked on that code :) Wim might also have some opinions on this.
Also related: https://bugzilla.gnome.org/show_bug.cgi?id=711816#c3 Maybe that helps?
No, it does not. I thought I could not reproduce my issue, but in fact it already does not happen when playing the same file (or, I guess, files with the same raw format). When playing files with different formats, I get the pause, with or without Jan's patch.
Not Jan's patch, that's just doing something related :) But his comment. I think that explains quite good what the problem is and how it should be solved.
Ah. I'm trying to fix it in baseaudiosink atm.
Created attachment 266465 [details] [review] do not lose track of position on caps change Fixes the issue in alsasink, while not breaking it in pulsesink. Note: I have no idea what I'm doing. I got it to work by poking repeatedly at what seemed vaguely relevant. This code is obscure to me. I hope I'm not breaking another particular case...
Comment on attachment 266465 [details] [review] do not lose track of position on caps change This was introduced by 98f41f1c390e22269f3042adda29d3d345699016 and/or 587b2721c876ca70b16f568c0ce12d8675d606ca btw. Which is fixing bug #697723. It might also make sense to revert those commits and fix things differently. Otherwise this patch seems to make sense but please also discuss with Wim before pushing.
Vincent, what's the status here?
Status is "the patches seem to work, they're being used in a client's product, but I don't know the surrounding code enough to be 100% sure it's all fine to push".
Did you ask Wim about his opinion already?
Hmm, I can't recall. I'll ask now as I don't see him commenting here.
wtay says the caps reset should be accompanied with a call to gst_audio_clock_reset. However, where to call this is not clear: 11:29 < wtay> v_, I think someone needs to call gst_audio_clock_reset() like what pulsesink does 11:33 < wtay> v_, not sure who.. maybe audio_sink ? 11:33 < wtay> v_, I don't want it to happen in baseaudiosink because some audio clocks don't reset in acquire/release (I know some use the system time) 11:37 < wtay> v_, it's all a bit suboptimal.. any subclass can just plug in their own clock and you can't make any assumptions about it in the base classes.. I'm really not clear on how to do that best/correctly, but this would be another fix altogether, making my patch moot.
Comment on attachment 266465 [details] [review] do not lose track of position on caps change See wtay's comments from IRC about resetting the clock instead.
What's the status here?
Re-reading, it looks like my patch is not the best way to fix this. The issue I originally had was on a previous client's hardware, where my patch fixed the issue. Maybe I should just reject my patch here.
What should happen with this bug then?
I'll close it then.