GNOME Bugzilla – Bug 739289
basesink: Stepping incorrectly updates start time when stepping in PAUSED, causing step amount to be calculated wrongly
Last modified: 2017-03-03 14:36:15 UTC
I recently came across an issue when using an MP4 File with H.264 Video (I-Frame only) without audio stream the step events are not executed as expected. It seems that the video "steps" to the current running-time. As longer i wait for the step the more further it steps in the video. For reference i tested this also with the gst-plugins-base/tests/examples/playback example which behaves exactly the same. Anyway, when using an MP4File with Video+Audio everything works expected and i can step frame by frame. Please let me know if i should attach test materials. Cheers, René
Created attachment 308309 [details] [review] BugFix-739289-update-start-time-only-in-playing-state Update start time when in playing state, because clock is available only in playing state
Fix works. Confirmed. Patch needs better formatting but I can do that when I merge. Since this is in GstBaseSink; will wait for the approval of a second reviewer.
For anybody wanting to reproduce it. I have some code to do this in a duplicate bug: https://bugzilla.gnome.org/show_bug.cgi?id=750283
Review of attachment 308309 [details] [review]: ::: libs/gst/base/gstbasesink.c @@ +3882,3 @@ sink->need_preroll = FALSE; + + if (GST_STATE (sink) == GST_STATE_PLAYING) I'm worried about locking here. Also, why isn't this check needed for _flush_start() ?
> I'm worried about locking here. Also, why isn't this check needed for > _flush_start() ? _update_start_time() in _flush_start() is not effecting seek, because there is a reset-time happening in _flush_stop(). But it is good to avoid _update_start_time() in _flush_start() when in PLAYING state.
Any update on this issue? We would like to see this patch in the official gstreamer release.
Nicolas?
Review of attachment 308309 [details] [review]: ::: libs/gst/base/gstbasesink.c @@ +3882,3 @@ sink->need_preroll = FALSE; + + if (GST_STATE (sink) == GST_STATE_PLAYING) I have checked the code, you need the object lock to do this check. Please fix, this is not thread safe. It's probably racy too.
Somehow it's linked to bug #744040, which holds the state lock while sending event. The check if correct "if" and only "if" the lock is held, but I doubt this is intended to be the case.
Yes, that lock should go away. I'm currently working on trying to find a solution for that, a bit tricky :)
*** Bug 750283 has been marked as a duplicate of this bug. ***
commit b9a4a2a95276d28aaa0e105d834e6f10d6e49d6f Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Jun 13 18:33:27 2016 +0200 basesink: Update start time when losing state only if we were in PLAYING If we were in PAUSED, the current clock time and base time don't have much to do with the running time anymore as the clock might have advanced while we were PAUSED. The system clock does that for example, audio clocks often don't. Updating the start time in PAUSED will cause a) the wrong position to be reported, b) step events to step not just the requested amount but the amount of time we spent in PAUSED. The start time should only ever be updated when going from PLAYING to PAUSED to remember the current running time (to be able to compensate later when going to PLAYING for the clock time advancing while PAUSED), not when we are already in PAUSED. Based on a patch by Kishore Arepalli <kishore.arepalli@gmail.com> The updating of the start time when the state is lost was added in commit ba943a82c0bbfd17c9ee9f5068d44c9d9274fd13 to fix the position reporting when the state is lost. This still works correctly after this change. https://bugzilla.gnome.org/show_bug.cgi?id=739289
I tested & use this patch on 1.4.3, it works fine. Cherry-pick b9a4a2a95276d28aaa0e105d834e6f10d6e49d6f on 1.4 git branch would be nice for posterity!