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 750027 - concat: Reset internal start offset to 0 after flushing seek
concat: Reset internal start offset to 0 after flushing seek
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 1.5.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-28 12:31 UTC by Carlos Rafael Giani
Modified: 2015-07-16 08:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for resetting the start offset after flushing seek (2.87 KB, patch)
2015-05-28 12:31 UTC, Carlos Rafael Giani
none Details | Review
Patch for resetting the start offset after flushing seek , v2 (2.91 KB, patch)
2015-05-28 12:35 UTC, Carlos Rafael Giani
none Details | Review
Updated patch to reset if FLUSH_STOP reset_time is TRUE (1.50 KB, patch)
2015-06-11 08:55 UTC, Carlos Rafael Giani
none Details | Review
Updated patch to only look at FLUSH_STOP from active pad (1.59 KB, patch)
2015-06-11 08:59 UTC, Carlos Rafael Giani
committed Details | Review

Description Carlos Rafael Giani 2015-05-28 12:31:00 UTC
Created attachment 304149 [details] [review]
Patch for resetting the start offset after flushing seek

This fixes seek problems that occur after concat switched to a next stream.
In such cases, concat will produce a segment with a nonzero base value.
This way, segments from individual streams are concatenated seamlessly.

However, after a flushing seek, the element base-time is reset. This means
that current_start_offset has to be reset to 0, since it was calculated
with the old base-time in mind.

This patch depends on https://bugzilla.gnome.org/show_bug.cgi?id=745366 .
Comment 1 Carlos Rafael Giani 2015-05-28 12:35:28 UTC
Created attachment 304152 [details] [review]
Patch for resetting the start offset after flushing seek , v2

Revised version which fixes indentation issues
Comment 2 Sebastian Dröge (slomo) 2015-05-29 19:42:06 UTC
Review of attachment 304152 [details] [review]:

::: plugins/elements/gstconcat.c
@@ +523,3 @@
+            "resetting start offset to 0 after flushing seek");
+        self->current_start_offset = 0;
+        self->reset_ofs_after_flushseek = FALSE;

I think resetting the base time makes sense *always* (not only after seeks) when we forward flush events. The sink will have its segment reset, so base starts from 0 again.

@@ +593,3 @@
+          GST_DEBUG_OBJECT (self,
+              "flushing seek detected; will reset start offset after flush");
+          self->reset_ofs_after_flushseek = TRUE;

To reliably match a flush event to the seek event you'll have to compare the sequence numbers
Comment 3 Carlos Rafael Giani 2015-06-01 13:10:12 UTC
In an earlier version of the patch I did reset the segment base with any flush event, not just seek-related ones. However, the docs only say that the base time is reset after a flushing seek. They do not state if it is reset after *every* kind of flush.
Comment 4 Sebastian Dröge (slomo) 2015-06-01 13:14:46 UTC
Every FLUSH_STOP that has reset_time==TRUE, it's independent of any seek events.
Comment 5 Sebastian Dröge (slomo) 2015-06-10 09:48:34 UTC
Ping? :)
Comment 6 Sebastian Dröge (slomo) 2015-06-10 09:56:47 UTC
Basically in all the forward==TRUE cases on FLUSH_STOP (with reset_time==TRUE inside the event) it should reset the time. And just not do anything specific about seek events or something
Comment 7 Carlos Rafael Giani 2015-06-11 08:55:17 UTC
Created attachment 305051 [details] [review]
Updated patch to reset if FLUSH_STOP reset_time is TRUE

Updated the patch. It is no longer flush-seek specific, and only needs the reset_time value from FLUSH_STOP.
Comment 8 Sebastian Dröge (slomo) 2015-06-11 08:58:40 UTC
Review of attachment 305051 [details] [review]:

::: plugins/elements/gstconcat.c
@@ +525,3 @@
+        GST_DEBUG_OBJECT (self,
+            "resetting start offset to 0 after flushing with reset_time = TRUE");
+        self->current_start_offset = 0;

I think this should only be done if the FLUSH_STOP event came from the active pad, not from some random other pad. For other pads we don't care really


We should also do the same in the src_event function
Comment 9 Carlos Rafael Giani 2015-06-11 08:59:21 UTC
Created attachment 305052 [details] [review]
Updated patch to only look at FLUSH_STOP from active pad

Second update. Only look at FLUSH_STOP if it comes from the active pad (forward==TRUE).
Comment 10 Sebastian Dröge (slomo) 2015-06-11 09:06:59 UTC
Hrm, forgot to ammend your commit before pushing. Please don't put the "Signed-off-by" noise into commit messages, and instead put a link to the bug report there :) git-bz does that automatically for you.

commit d8db88d078dc2688858ec6393379ade6b5990771
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Jun 11 11:05:53 2015 +0200

    concat: Also reset the current start offset when receiving a FLUSH_STOP on the srcpad

commit c324e31c26e7eb088ac79001e4eb5ed76fe8f094
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Jun 11 11:05:38 2015 +0200

    concat: Add some newlines to event handling code to make the code look a bit less dense

commit 25c66e60045e44d50a591f5d62706f6f243bb9ca
Author: Carlos Rafael Giani <dv@pseudoterminal.org>
Date:   Thu Jun 11 10:53:30 2015 +0200

    concat: Reset segment base offset after FLUSH_STOP with reset_time = TRUE
    
    If the reset_time value of a FLUSH_STOP event is set to TRUE, the pipeline
    will have the base_time of its elements reset. This means that the concat
    element's current_start_offset has to be reset to 0, since it was
    calculated with the old base-time in mind.
    
    Only FLUSH_STOP events coming from the active pad are looked at.
    
    Signed-off-by: Carlos Rafael Giani <dv@pseudoterminal.org>
Comment 11 David Griffin 2015-07-16 07:03:38 UTC
A similar problem seems to exist with the latest versions of GStreamer git master. The problem was fixed for a short period of time, but now seeks are inaccurate again when using a concat element in a pipeline. Any seek I perform one a pipeline with a concat element seeks the pipeline to a position before the time I request. Playing or Paused.
Comment 12 Carlos Rafael Giani 2015-07-16 08:51:31 UTC
(In reply to David Griffin from comment #11)
> A similar problem seems to exist with the latest versions of GStreamer git
> master. The problem was fixed for a short period of time, but now seeks are
> inaccurate again when using a concat element in a pipeline. Any seek I
> perform one a pipeline with a concat element seeks the pipeline to a
> position before the time I request. Playing or Paused.

Can you add some details about your pipeline? It would make replicating the issue easier.