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 676739 - Make stream changes using input-selector/playbin2 instantaneous
Make stream changes using input-selector/playbin2 instantaneous
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal normal
: 0.10.37
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-05-24 13:44 UTC by Andre Moreira Magalhaes
Modified: 2012-05-31 10:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstinputselector: Properly sync when changing streams. (30.51 KB, patch)
2012-05-24 13:45 UTC, Andre Moreira Magalhaes
needs-work Details | Review
gstplaybin2: Also send flush events when changing audio (18.68 KB, patch)
2012-05-24 13:46 UTC, Andre Moreira Magalhaes
none Details | Review
gstassrender: Refactoring. (31.79 KB, patch)
2012-05-24 13:48 UTC, Andre Moreira Magalhaes
needs-work Details | Review
gstplaybin2: Also send flush events when changing audio/video tracks. (24.40 KB, patch)
2012-05-25 16:12 UTC, Andre Moreira Magalhaes
reviewed Details | Review
gstinputselector: Properly sync when changing streams. (38.34 KB, patch)
2012-05-28 17:54 UTC, Andre Moreira Magalhaes
none Details | Review
gstinputselector: Add sync-mode and cache-buffers properties. (12.72 KB, patch)
2012-05-28 17:56 UTC, Andre Moreira Magalhaes
needs-work Details | Review
gstplaybin2: Also send flush events when changing audio/video tracks. (25.11 KB, patch)
2012-05-28 17:58 UTC, Andre Moreira Magalhaes
committed Details | Review
gsttextoverlay: Use external lock. (15.18 KB, patch)
2012-05-28 17:59 UTC, Andre Moreira Magalhaes
committed Details | Review
gstassrender: Refactoring. (33.68 KB, patch)
2012-05-28 18:01 UTC, Andre Moreira Magalhaes
committed Details | Review
gstinputselector: Properly sync when changing streams. (37.79 KB, patch)
2012-05-30 17:50 UTC, Andre Moreira Magalhaes
committed Details | Review
gstplaysink: Do not send flush events for audio/video and use the old input-selector mode for them. (4.63 KB, patch)
2012-05-30 17:54 UTC, Andre Moreira Magalhaes
committed Details | Review

Description Andre Moreira Magalhaes 2012-05-24 13:44:26 UTC
+++ This bug was initially created as a clone of Bug #638168 +++

The original bug was intended to fix a bug in textoverlay and ended up being a general fix for stream changes, so cloning into this bug.
Comment 1 Andre Moreira Magalhaes 2012-05-24 13:45:51 UTC
Created attachment 214859 [details] [review]
gstinputselector: Properly sync when changing streams.

Same as https://bugzilla.gnome.org/show_bug.cgi?id=638168#c57
Comment 2 Andre Moreira Magalhaes 2012-05-24 13:46:41 UTC
Created attachment 214860 [details] [review]
gstplaybin2: Also send flush events when changing audio

Same as https://bugzilla.gnome.org/show_bug.cgi?id=638168#c58
Comment 3 Andre Moreira Magalhaes 2012-05-24 13:48:18 UTC
Created attachment 214861 [details] [review]
gstassrender: Refactoring.

Same as https://bugzilla.gnome.org/show_bug.cgi?id=638168#c59
Comment 4 Andre Moreira Magalhaes 2012-05-25 16:12:16 UTC
Created attachment 214960 [details] [review]
gstplaybin2: Also send flush events when changing audio/video tracks.

New patch replacing patch from comment #2 with some refactoring and also doing the flush dance for video streams.

Still working with all tested videos:

For subtitles change:
- http://docs.gstreamer.com/media/sintel_trailer-480p.ogv with the suburi
http://docs.gstreamer.com/media/sintel_trailer_gr.srt
- http://matroska.free.fr/samples/mewmew/downloads/mewmew-vorbis-ssa.mkv
- http://www.gnu.org/fry/happy-birthday-to-gnu-download.html

For audio change:
- http://docs.gstreamer.com/media/sintel_cropped_multilingual.webm

And for video change, using the video generated with:
- gst-launch-0.10 videotestsrc num-buffers=1000 ! "video/x-raw-yuv, width=640, height=480" ! timeoverlay ! theoraenc ! oggmux name=mux ! filesink location=test.ogv    videotestsrc num-buffers=1000 pattern=snow ! "video/x-raw-yuv, width=640, height=480" ! timeoverlay ! theoraenc ! mux.
Comment 5 Sebastian Dröge (slomo) 2012-05-28 08:41:46 UTC
Review of attachment 214861 [details] [review]:

::: ext/assrender/gstassrender.c
@@ +85,3 @@
 
+#define GST_ASS_RENDER_GET_COND(ass)  (((GstAssRender *)ass)->subtitle_cond)
+#define GST_ASS_RENDER_WAIT(ass)      (g_cond_wait (GST_ASS_RENDER_GET_COND (ass), GST_OBJECT_GET_LOCK (ass)))

Don't use the object lock for things like this, if anything it should only be used for protecting very small parts of code and definitely not for a condition variable. As this mutex is shared it is too easy to get deadlocks and other weird behaviour otherwise. Please use a private mutex for this

@@ +448,3 @@
     }
     default:
+      if (render->track_init_ok) {

Why don't you push events always on both pads?

@@ +1189,3 @@
+      if (wait_for_text_buf) {
+        GST_DEBUG_OBJECT (render, "no text buffer, need to wait for one");
+        GST_ASS_RENDER_WAIT (render);

If I understand this correctly you *always* wait for a text buffer now unless you saw one that is after the current video position? This is going to break as text streams are sparse. And waiting for text buffers if the segment format is not TIME is dangerous too

If you always wait on text buffers you have to take the filler newsegment events into account (the ones with update=TRUE that just advance the start position of the segment). textoverlay's wait-for-text mode does this IIRC, check with that. (And it makes sense to set wait-for-text to TRUE on all elements inside subtitleoverlay if they have such a property btw)

@@ +1269,3 @@
   }
 
+  if (G_LIKELY (GST_BUFFER_TIMESTAMP_IS_VALID (buffer))) {

We don't require valid timestamps on ASS buffers anymore? Without we can't tell libass when to render something... it should be safe to require all buffers to have timestamps here as before

@@ +1515,3 @@
 
+      GST_OBJECT_LOCK (render);
+      render->subtitle_eos = FALSE;

The EOS state only has to be cleared when receiving flush-stop
Comment 6 Sebastian Dröge (slomo) 2012-05-28 09:29:31 UTC
Review of attachment 214960 [details] [review]:

::: gst/playback/gstplaysink.c
@@ +1902,3 @@
+
+    custom_flush = g_strdup_printf ("custom-%s-flush", sink_type);
+    custom_flush_finish = g_strdup_printf ("custom-%s-flush-finish", sink_type);

Maybe just hardcode the three different names here instead of doing the dynamic string dance :)
I would suggest prefixing the event names with playsink as it's now playsink API.
Comment 7 Sebastian Dröge (slomo) 2012-05-28 09:40:32 UTC
Review of attachment 214859 [details] [review]:

Please restore the old syncing behaviour based on the buffer timestamps and current segments (add a property to select between clock and timestamp and make timestamp the default), and make the maximum cache size (in time and bytes) configurable (i.e. -1 current behaviour, 0 no caching, any other value cache at most that much).

::: plugins/elements/gstinputselector.c
@@ +177,3 @@
+  GstBuffer *buffer;
+  GstSegment segment;
+  gboolean segment_update;

Why do you keep a copy of the segment again?

@@ +406,3 @@
+  GST_DEBUG_OBJECT (selpad, "Caching buffer %p", buffer);
+  if (!selpad->cached_buffers)
+    selpad->cached_buffers = g_queue_new ();

You could also directly store a GQueue inside the selpad struct instead of needing to allocate one

@@ +1389,3 @@
+  g_signal_connect_data (sel, "notify::active-pad",
+      (GCallback) gst_input_selector_active_pad_changed, NULL,
+      NULL, G_CONNECT_AFTER);

Why do you connect to your own signal here? With a NULL callback
Comment 8 Andre Moreira Magalhaes 2012-05-28 11:59:14 UTC
(In reply to comment #7)
> Review of attachment 214859 [details] [review]:
> 
> Please restore the old syncing behaviour based on the buffer timestamps and
> current segments (add a property to select between clock and timestamp and make
> timestamp the default), and make the maximum cache size (in time and bytes)
> configurable (i.e. -1 current behaviour, 0 no caching, any other value cache at
> most that much).
Yep, I am on it.

> ::: plugins/elements/gstinputselector.c
> @@ +177,3 @@
> +  GstBuffer *buffer;
> +  GstSegment segment;
> +  gboolean segment_update;
> 
> Why do you keep a copy of the segment again?
The CachedBuffer keeps the segment for that buffer and whether it is a segment update or not, to be used when this pad is reactivated.

> @@ +406,3 @@
> +  GST_DEBUG_OBJECT (selpad, "Caching buffer %p", buffer);
> +  if (!selpad->cached_buffers)
> +    selpad->cached_buffers = g_queue_new ();
> 
> You could also directly store a GQueue inside the selpad struct instead of
> needing to allocate one
I prefer not to, as the queue (cached buffers) is only created for pads that were once active. There is no need to allocate a gqueue if we are never using it and also I prefer checking for null instead of checking the size of the queue. That will be even more useful now that cached buffers will be optional.
 
> @@ +1389,3 @@
> +  g_signal_connect_data (sel, "notify::active-pad",
> +      (GCallback) gst_input_selector_active_pad_changed, NULL,
> +      NULL, G_CONNECT_AFTER);
> 
> Why do you connect to your own signal here? With a NULL callback
This is not an empty callback. The callback called is gst_input_selector_active_pad_changed and it is called after all other callbacks connected to this signal. What happens is that I want playbin2 and others to have a chance to do something after the pad changes before input-selector proceeds. "notify::active-pad" is emitted automatically when set_property returns, and playbin2 catches the signal and mark the bin for flush as appropriate.
Comment 9 Andre Moreira Magalhaes 2012-05-28 12:02:12 UTC
(In reply to comment #6)
> Review of attachment 214960 [details] [review]:
> 
> ::: gst/playback/gstplaysink.c
> @@ +1902,3 @@
> +
> +    custom_flush = g_strdup_printf ("custom-%s-flush", sink_type);
> +    custom_flush_finish = g_strdup_printf ("custom-%s-flush-finish",
> sink_type);
> 
> Maybe just hardcode the three different names here instead of doing the dynamic
> string dance :)
> I would suggest prefixing the event names with playsink as it's now playsink
> API.
I prefer not to tbh, that would make 6 different variables and I don't want to check for text-flush on audio/video events, etc. I left the names without a prefix because they are also used in subtitleoverlay (custom-text-flush only), but I can change if you prefer
Comment 10 Andre Moreira Magalhaes 2012-05-28 17:54:48 UTC
Created attachment 215140 [details] [review]
gstinputselector: Properly sync when changing streams.

This patch is the same as patch from comment #1 but with sync-mode and cache-buffers properties added. With the properties set to default values (sync-mode=active-segment and cache-buffers=FALSE), input-selector should behave as currently (without any patches).
Comment 11 Andre Moreira Magalhaes 2012-05-28 17:56:49 UTC
Created attachment 215141 [details] [review]
gstinputselector: Add sync-mode and cache-buffers properties.

This patch is the diff between patch from comment #1 and patch from comment #10, for easier review.
Comment 12 Andre Moreira Magalhaes 2012-05-28 17:58:44 UTC
Created attachment 215142 [details] [review]
gstplaybin2: Also send flush events when changing audio/video tracks.

This patch is the same as patch from comment #4 with the following changes:
- Add a playsink prefix to custom events.
- Set the new properties of input-selector to cache-mode=TRUE and sync-mode=clock
Comment 13 Andre Moreira Magalhaes 2012-05-28 17:59:23 UTC
Created attachment 215143 [details] [review]
gsttextoverlay: Use external lock.
Comment 14 Andre Moreira Magalhaes 2012-05-28 18:01:35 UTC
Created attachment 215144 [details] [review]
gstassrender: Refactoring.

This patch is the same as patch from comment #3 but using an external lock instead.

As explained over IRC, the changes here are mostly a verbatim copy of pango/textoverlay with the needed adjustments to make it work.

If we are going to change something here I suggest we do the same in pango/textoverlay too.
Comment 15 Andre Moreira Magalhaes 2012-05-28 18:04:31 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > Review of attachment 214859 [details] [review] [details]:
> > 
> > Please restore the old syncing behaviour based on the buffer timestamps and
> > current segments (add a property to select between clock and timestamp and make
> > timestamp the default), and make the maximum cache size (in time and bytes)
> > configurable (i.e. -1 current behaviour, 0 no caching, any other value cache at
> > most that much).
> Yep, I am on it.

As agreed over IRC, I've added the changes to select the sync-mode (clock or active segment) and to disable the cache, but there is no property for the maximum cache size as the way it is implemented there is no point in adding one.

The active pad is the only one with cached buffers and when becoming inactive, the first call to chain will consume the cached buffers for such pad. While active the buffers are discarded as soon as they pass the current running time, thus never growing much.
Comment 16 Andre Moreira Magalhaes 2012-05-28 18:05:39 UTC
... and all tested videos from comment #4 still work with the latest patches.
Comment 17 Sebastian Dröge (slomo) 2012-05-29 09:18:00 UTC
Review of attachment 215141 [details] [review]:

::: plugins/elements/gstinputselector.c
@@ +766,3 @@
+      clock = gst_element_get_clock (GST_ELEMENT_CAST (sel));
+      if (clock)
+        cur_running_time = gst_clock_get_time (clock);

You need to take the element's base time into account here. A buffer of running time T is to be shown if base_time + clock_time reaches T. So you need to subtract the base_time from the clock time here (and make sure that the clock time is larger than the base time, otherwise just use 0).

@@ +1590,3 @@
     /* schedule a new segment push */
+    if (self->sync_streams
+        && self->sync_mode == GST_INPUT_SELECTOR_SYNC_MODE_CLOCK)

Why is this only done in clock sync-mode?
Comment 18 Andre Moreira Magalhaes 2012-05-30 17:50:16 UTC
Created attachment 215272 [details] [review]
gstinputselector: Properly sync when changing streams.

This patch is the same as the one from comment #12, with the following additions:
- Keep full compatibility with old behaviour when sync-mode=active-seg and cache-buffers=FALSE
- Consider base_time when deciding whether to discard buffers
- Fix a bug when flushing and using cached buffers, where seek could fail in an endless loop
Comment 19 Andre Moreira Magalhaes 2012-05-30 17:52:27 UTC
(In reply to comment #18)
> Created an attachment (id=215272) [details] [review]
> gstinputselector: Properly sync when changing streams.
> 
> This patch is the same as the one from comment #12, with the following
> additions:
*Comment #11 actually.
Comment 20 Andre Moreira Magalhaes 2012-05-30 17:54:17 UTC
Created attachment 215273 [details] [review]
gstplaysink: Do not send flush events for audio/video and use the old input-selector mode for them.

This patch is on top of patch from comment #12 which was already accepted, but commenting out the flushes and using the old input-selector behaviour for audio and video.
Comment 21 Sebastian Dröge (slomo) 2012-05-31 10:43:11 UTC
Pushed them all, now just needs to be ported to 0.11 with all the other recent changes here.