Assuming seek goes to position T and the video keyframe is at position T_k, the problem here is that:
- video plays from T_k, which gets running time 0 because of a flushing seek
- audio does the same because it comes from the same demuxer
- subs play from T, which gets running time 0 because of flushing seeks
- sub / video, and sub / audio sync is off by T_k - T

Correct?


I think the solution here would be something inside playbin that adjusts the segment events coming from external subtitle files to match up with the audio/video segments. It can probably be solved by comparing the stream times of both streams and the corresponding running times. Stream time should be the same on both, that's what the seek event controls... and running times of both streams should be the same at the same stream time, otherwise sync is off.

Also I don't think this should be done in streamsynchronizer, at least not without additional signalling from playbin somehow. It seems too playbin specific.

So the solution should be below:
1. playbin add pad probe function to probe event from demuxer source pad and subparse source pad.
2. hold segment event until received all segment event.
3. select the least segment.time and segment.start.
4. replace all segment event with the least segment.time and segment.start.

streamsynchronizer already has hold and wait segment function. But the wait only take effect when received segment start event.

Add more information for the issue:

The issue only occur when application set appsink to playbin to sink subtitle. No such kind of issue if combine subtitle with video. Subtitle pipeline segment can't reach basesink as subtitle combined with video, so no such kind of issue.

I do see the issue on Gstreamer 1.4.1. Not sure if fixed this kind of issue on latest code.

(In reply to Sebastian Dröge (slomo) from comment #1)
> Assuming seek goes to position T and the video keyframe is at position T_k,
> the problem here is that:
> - video plays from T_k, which gets running time 0 because of a flushing seek
> - audio does the same because it comes from the same demuxer
> - subs play from T, which gets running time 0 because of flushing seeks
> - sub / video, and sub / audio sync is off by T_k - T
> 
> Correct?
> 
Yes, you are right.
> 
> I think the solution here would be something inside playbin that adjusts the
> segment events coming from external subtitle files to match up with the
> audio/video segments. It can probably be solved by comparing the stream
> times of both streams and the corresponding running times. Stream time
> should be the same on both, that's what the seek event controls... and
> running times of both streams should be the same at the same stream time,
> otherwise sync is off.

Where is the place to change the segment? playbin?

In playbin between the subtitle uridecodebin and the input-selector for subtitles

I will try it and upload patch for review. Thanks.

Created attachment 301147 [details] [review]
Draft patch for the issue.

Waiting for review...

Ping2....

Review of attachment 301147 [details] [review]:

Please describe in more detail what this is supposed to fix, and in which situation this problem can appear

::: gst/playback/gstplaybin2.c
@@ +3055,3 @@
+
+        GST_SOURCE_GROUP_LOCK (group);
+        _uridecodebin_event_probe_wait (group);

I think it's cleaner to use a blocking pad probe for this instead of using a custom cond. Especially a block pad probe will handle things properly for flushing and pad deactivation

@@ +3058,3 @@
+        if (segment.format == GST_FORMAT_TIME) {
+          segment.start = group->nonsub_segment.start;
+          segment.time = group->nonsub_segment.time;

Why only these two? Should probably replace the whole segment

@@ +5322,3 @@
       }
       gst_element_set_state (suburidecodebin, GST_STATE_READY);
+      g_cond_init (&group->suburidecodebin_segment_cond);

Here you would init the cond on every group switch... it should only be done exactly once when playbin is created (and then cleared in finalize)

@@ +5500,3 @@
+    group->got_nonsub_segment = TRUE;
+    g_cond_broadcast (&group->suburidecodebin_segment_cond);
+    g_cond_clear (&group->suburidecodebin_segment_cond);

Especially this will probably cause problems as at this time another thread is actually still using the cond

(In reply to Sebastian Dröge (slomo) from comment #11)
> Review of attachment 301147 [details] [review] [review]:
> 
> Please describe in more detail what this is supposed to fix, and in which
> situation this problem can appear
> 

External subtitle don't sync with Audio/Video after seek. The root cause is external subtitle's segment is different with Audio/Video's segment after seek. The solution is replace subtitle's segment with Audio/Video's segment.

> ::: gst/playback/gstplaybin2.c
> @@ +3055,3 @@
> +
> +        GST_SOURCE_GROUP_LOCK (group);
> +        _uridecodebin_event_probe_wait (group);
> 
> I think it's cleaner to use a blocking pad probe for this instead of using a
> custom cond. Especially a block pad probe will handle things properly for
> flushing and pad deactivation
> 

Accept. Can reuse function gst_play_bin_suburidecodebin_block() to block the subtitle segment event? When we should add the block probe? Add the block probe when playbin received seed event?

> @@ +3058,3 @@
> +        if (segment.format == GST_FORMAT_TIME) {
> +          segment.start = group->nonsub_segment.start;
> +          segment.time = group->nonsub_segment.time;
> 
> Why only these two? Should probably replace the whole segment
> 
Accept.
> @@ +5322,3 @@
>        }
>        gst_element_set_state (suburidecodebin, GST_STATE_READY);
> +      g_cond_init (&group->suburidecodebin_segment_cond);
> 
> Here you would init the cond on every group switch... it should only be done
> exactly once when playbin is created (and then cleared in finalize)
> 
Needn't custom cond any more.
> @@ +5500,3 @@
> +    group->got_nonsub_segment = TRUE;
> +    g_cond_broadcast (&group->suburidecodebin_segment_cond);
> +    g_cond_clear (&group->suburidecodebin_segment_cond);
> 
> Especially this will probably cause problems as at this time another thread
> is actually still using the cond
Needn't custom cond any more.

I think you forgot to attach the updated patch :)

Created attachment 303378 [details] [review]
Update patch based on review comments.

Created attachment 303379 [details] [review]
Update patch based on review comments.

(In reply to kevin from comment #4)
> Add more information for the issue:
> 
> The issue only occur when application set appsink to playbin to sink
> subtitle. No such kind of issue if combine subtitle with video. Subtitle
> pipeline segment can't reach basesink as subtitle combined with video, so no
> such kind of issue.

This sounds like something would be wrong in subtitleoverlay/assrender/textoverlay/etc too then. They should show the same behaviour as a text-sink.




Also I think your patch is going to fail if the audio/video upstream is setting a different base time, or otherwise has a non-standard segment. I think we need to do something more clever here by actually not copying the segment... but instead making sure that the subtitle segment is based on the audio/video segment in a way that the first subtitle timestamp inside the segment is the keyframe timestamp.

For this you would have to a) catch the audio/video segment and remember it, b) *modify* the subtitle segment to adjust the start (and possibly other fields) of the segment. The resulting subtitle segment should cause all subs to be before the segment until the keyframe timestamp, the keyframe timestamp should be mapped to running time 0 and to the stream time that the keyframe timestamp also has for the audio/video segment.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/159.