[Bug 676739] Make stream changes using input-selector/playbin2 instantaneous

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=676739
  GStreamer | gst-plugins-base | unspecified

Sebastian Dröge <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #214861|none                        |needs-work
             status|                            |

--- Comment #5 from Sebastian Dröge <slomo at circular-chaos.org> 2012-05-28 08:41:46 UTC ---
Review of attachment 214861:
 --> (https://bugzilla.gnome.org/review?bug=676739&attachment=214861)

::: 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

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.