After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 739289 - basesink: Stepping incorrectly updates start time when stepping in PAUSED, causing step amount to be calculated wrongly
basesink: Stepping incorrectly updates start time when stepping in PAUSED, ca...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.4.3
Other Linux
: Normal normal
: 1.8.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 750283 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-10-28 11:21 UTC by René Calles
Modified: 2017-03-03 14:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
BugFix-739289-update-start-time-only-in-playing-state (1.13 KB, patch)
2015-07-28 15:39 UTC, Kishore Arepalli
committed Details | Review

Description René Calles 2014-10-28 11:21:47 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é
Comment 1 Kishore Arepalli 2015-07-28 15:39:10 UTC
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
Comment 2 Luis de Bethencourt 2015-07-30 13:03:16 UTC
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.
Comment 3 Luis de Bethencourt 2015-07-30 13:19:05 UTC
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
Comment 4 Nicolas Dufresne (ndufresne) 2015-07-30 14:42:48 UTC
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() ?
Comment 5 Kishore Arepalli 2015-07-30 15:10:50 UTC
> 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.
Comment 6 Zack Snyder 2016-01-28 12:56:54 UTC
Any update on this issue? We would like to see this patch in the official gstreamer release.
Comment 7 Sebastian Dröge (slomo) 2016-02-17 14:24:37 UTC
Nicolas?
Comment 8 Nicolas Dufresne (ndufresne) 2016-02-22 19:53:54 UTC
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.
Comment 9 Nicolas Dufresne (ndufresne) 2016-02-22 19:57:43 UTC
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.
Comment 10 Sebastian Dröge (slomo) 2016-02-22 21:59:31 UTC
Yes, that lock should go away. I'm currently working on trying to find a solution for that, a bit tricky :)
Comment 11 Sebastian Dröge (slomo) 2016-06-12 20:10:47 UTC
*** Bug 750283 has been marked as a duplicate of this bug. ***
Comment 12 Sebastian Dröge (slomo) 2016-06-13 19:38:28 UTC
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
Comment 13 Matthieu Crapet 2017-03-03 14:36:15 UTC
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!