GNOME Bugzilla – Bug 749258
basesink: fix QoS/lateness checking if subclass implements prepare/prepare_list vfuncs
Last modified: 2015-05-14 08:20:12 UTC
Created attachment 303252 [details] [review] patch In basesink functions gst_base_sink_chain_unlocked(), below code is used to checking if buffer is late before doing prepare call to save some effort: if (syncable && do_sync) late = gst_base_sink_is_too_late (basesink, obj, rstart, rstop, GST_CLOCK_EARLY, 0, FALSE); if (G_UNLIKELY (late)) goto dropped; But this code has problem, it should calculate jitter based on current media clock, rather than just passing 0. I found it will drop all the frames when rewind in slow speed, such as -2X. I made a patch to fix this issue, please check it.
This is probably related to bug #742916, can you confirm?
Looks like this patch do the sync waiting (with gst_base_sink_do_sync) before prepare call, will this cause buffer late in the next gst_base_sink_do_sync() call, as it may cost some time from sync in prepare to sync in render? I think in prepare, just need to check if current buffer time is late than media time, if late drop it to save some prepare effort. I believe this is the original purpose of this code.
*** Bug 742916 has been marked as a duplicate of this bug. ***
Review of attachment 303252 [details] [review]: I agree, your patch makes more sense. ::: libs/gst/base/gstbasesink.c @@ +3367,3 @@ + base_time = GST_ELEMENT_CAST (basesink)->base_time; + stime = base_time + gst_base_sink_adjust_time (basesink, rstart); + now = gst_clock_get_time (clock); When accessing the clock and base time directly like this, you need to take the object lock all the time. @@ +3371,2 @@ gst_base_sink_is_too_late (basesink, obj, rstart, rstop, + GST_CLOCK_EARLY, GST_CLOCK_DIFF (stime, now), FALSE); Not sure if EARLY is always correct here... it might depend on whether stime>now or not, no?
I think EARLY is OK, it just reuse gst_base_sink_is_too_late() to check if the buffer is late, so use EARLY can hard code to make this function work. Please check the updated patch based on your comments.
Created attachment 303352 [details] [review] update patch
Thanks! I just moved the lock a little bit before merging. commit 4f79c5e8da516c137d4c9b0175ccb0a2ab147a6b Author: Jian <Jian.Li@freescale.com> Date: Thu May 14 15:49:43 2015 +0800 basesink: Fix QoS/lateness checking if subclass implements prepare/prepare_list vfuncs In basesink functions gst_base_sink_chain_unlocked(), below code is used to checking if buffer is late before doing prepare call to save some effort: if (syncable && do_sync) late = gst_base_sink_is_too_late (basesink, obj, rstart, rstop, GST_CLOCK_EARLY, 0, FALSE); if (G_UNLIKELY (late)) goto dropped; But this code has problem, it should calculate jitter based on current media clock, rather than just passing 0. I found it will drop all the frames when rewind in slow speed, such as -2X. https://bugzilla.gnome.org/show_bug.cgi?id=749258