GNOME Bugzilla – Bug 751147
appsink: pull_preroll returns wrong segment in the sample
Last modified: 2015-08-16 13:40:20 UTC
Created attachment 305540 [details] [review] Patch that solves the issue When appsink prerolls, the sample returned by pull_preroll() has a wrong segment in it. The actual segment sits and waits in appsink's internal queue until one calls pull_buffer() instead.
Review of attachment 305540 [details] [review]: ::: gst-libs/gst/app/gstappsink.c @@ +572,3 @@ GST_DEBUG_OBJECT (appsink, "receiving SEGMENT"); g_queue_push_tail (priv->queue, gst_event_ref (event)); + gst_event_copy_segment (event, &priv->preroll_segment); Doesn't this mean that it will always have the latest segment received? I think it should only consider the segment received before the preroll buffer, so it should only do this while preroll == NULL. For consistency it might also be a good idea to init it to UNDEFINED/TIME again when ins _stop
Indeed, it does mean that the latest segment will be received in preroll_segment. However, the same is done with preroll_caps and the preroll buffer never becomes NULL until dispose(), so checking for preroll==NULL would be problematic at this point. I guess the whole logic could be improved for optimization, but it works. Initializing it in stop() doesn't sound like it has any advantage to me. A preroll can only happen in PAUSED state, so if stop() gets called (going to NULL), then start() will be called after that before a new preroll. The only potential problem is what happens if somebody calls "pull-preroll" when appsink is in the NULL state, but that is broken anyway with the current code regarding the preroll variables (buffer & caps). I would not consider fixing this as part of this patch.
(In reply to George Kiagiadakis from comment #2) > Indeed, it does mean that the latest segment will be received in > preroll_segment. However, the same is done with preroll_caps and the preroll > buffer never becomes NULL until dispose(), so checking for preroll==NULL > would be problematic at this point. I guess the whole logic could be > improved for optimization, but it works. I'd more think of it as 'works' as you can get the wrong segment. But I agree this is an improvement anyway. > > Initializing it in stop() doesn't sound like it has any advantage to me. A > preroll can only happen in PAUSED state, so if stop() gets called (going to > NULL), then start() will be called after that before a new preroll. The only > potential problem is what happens if somebody calls "pull-preroll" when > appsink is in the NULL state, but that is broken anyway with the current > code regarding the preroll variables (buffer & caps). I would not consider > fixing this as part of this patch. The caps issue can be done as a separate patch/bug.
A unit test would be great too :)
Created attachment 309291 [details] [review] appsink: put the correct segment in the preroll sample
Created attachment 309292 [details] [review] tests/appsink: add test to ensure that the segment returned by pull-preroll/sample is correct
Created attachment 309293 [details] [review] appsink: do not update preroll_caps unless the sink is prerolling
Created attachment 309294 [details] [review] appsink: unref the preroll buffer and cleanup the segments on stop()
commit 897371ac4fb75e5d334702341dc641a0e96f26a2 Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Fri Aug 14 18:43:03 2015 +0200 appsink: unref the preroll buffer and cleanup the segments on stop() Just for consistency. No need to keep data around. commit c3e4d8ca6fe118869506065e46fee5fe6d5bcacc Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Fri Aug 14 18:35:22 2015 +0200 appsink: do not update preroll_caps unless the sink is prerolling Just for consistency with the preroll_segment commit 41cb26b0e9969d2b49c26a259bb3bb8a6e9217d6 Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Fri Aug 14 18:06:03 2015 +0200 tests/appsink: add test to ensure that the segment returned by pull-preroll/sample is correct https://bugzilla.gnome.org/show_bug.cgi?id=751147 commit c411819452524c281b58b531829aa3914de53ef2 Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Thu Jun 18 12:30:24 2015 +0200 appsink: put the correct segment in the preroll sample last_segment is only being updated in dequeue_buffer(), which is only called from _pull_sample(). _pull_preroll() simply re-uses an old or dummy segment while the actual one sits and waits in the queue. https://bugzilla.gnome.org/show_bug.cgi?id=751147