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 635001 - basesink: fix position reporting in PAUSED
basesink: fix position reporting in PAUSED
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.10.32
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-11-16 17:38 UTC by Andy Wingo
Modified: 2010-12-15 17:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gst_base_sink_get_position refactor (2.76 KB, patch)
2010-11-16 17:39 UTC, Andy Wingo
none Details | Review
fix position reporting in PAUSED (4.70 KB, patch)
2010-11-16 17:40 UTC, Andy Wingo
none Details | Review

Description Andy Wingo 2010-11-16 17:38:48 UTC
Currently, if you play a stream at e.g. 3x speed, query the position, pause the pipeline, then query the position again, the second position is earlier than the first. This is pretty bogus; in my app that results in time seemingly going backwards a bit when playback resumes after a flushing seek to adjust rate back to 1.0, but to the same timestamp.

The solution is to unify the logic for basesink position queries in PAUSED and PLAYING. In PAUSED instead of using the base time and the clock, given that we have neither, we use the start_time instead, in combination with the current segment.

The two attached patches implement this refactoring.
Comment 1 Andy Wingo 2010-11-16 17:39:42 UTC
Created attachment 174618 [details] [review]
gst_base_sink_get_position refactor
Comment 2 Andy Wingo 2010-11-16 17:40:03 UTC
Created attachment 174619 [details] [review]
fix position reporting in PAUSED
Comment 3 Wim Taymans 2010-11-16 17:58:09 UTC
+  if (oformat == tformat)
+    running_time = GST_ELEMENT_START_TIME (basesink);
+  else {
+    gint64 rt;
+
+    rt = gst_segment_to_running_time (segment, oformat, segment->last_stop);
+
+    /* convert values from segment to time */
+    if (!gst_pad_query_convert (basesink->sinkpad, oformat, accum, &tformat,
+            &accum))
+      goto convert_failed;
+    if (!gst_pad_query_convert (basesink->sinkpad, oformat, duration, &tformat,
+            &duration))
+      goto convert_failed;
+    if (!gst_pad_query_convert (basesink->sinkpad, oformat, time, &tformat,
+            &time))
+      goto convert_failed;
+    if (!gst_pad_query_convert (basesink->sinkpad, oformat, rt, &tformat, &rt))
+      goto convert_failed;
+
+    running_time = (rt < 0) ? 0 : rt;
   }

Even if the segment format is not in time, we can use the START_TIME of the element. The only reason you need to use the last_stop is when you don't have a valid start_time.
Comment 4 Andy Wingo 2010-11-17 11:12:42 UTC
What do you mean by "start_time"? segment.start?

This code only gets run when we actually have a new segment, and I don't see the case where segment.start could be -1. Did you mean segment.time?
Comment 5 Wim Taymans 2010-11-17 11:14:39 UTC
I meant GST_ELEMENT_START_TIME() in the above code it's not used when the segment format is not in GST_FORMAT_TIME.
Comment 6 Andy Wingo 2010-11-17 17:00:29 UTC
You said "The only reason you need to use the last_stop is when you don't have a
valid start_time", but the GST_ELEMENT_START_TIME() doesn't have an invalid marker; i.e. it's never -1, AFAIK.
Comment 7 Wim Taymans 2010-11-17 17:05:49 UTC
The start_time can be -1 when distribution of base_time is disabled with  gst_pipeline_set_new_stream_time() or when the application calls gst_element_set_start_time() with GST_CLOCK_TIME_NONE.
Comment 8 Andy Wingo 2010-12-15 17:50:07 UTC
Wim says this is fixed in the recent basesink patches, between 25 Nov and 14 Dec. Closing provisionally; will re-open if issue reappears.