[Bug 638168] textoverlay: don't return wrong-state when stopping while in text chain

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #47 from Sebastian Dröge <slomo at circular-chaos.org> 2012-05-21 07:19:21 UTC ---
Review of attachment 214509:
 --> (https://bugzilla.gnome.org/review?bug=638168&attachment=214509)

::: plugins/elements/gstinputselector.c
@@ +522,3 @@
          */
+        active_selpad = GST_SELECTOR_PAD (active_sinkpad);
+        forward = (active_selpad->eos && !active_selpad->eos_sent);

Why do you get the pad again here? As you hold the lock over the complete
switch statement the active pad couldn't have changed here

@@ -576,3 @@

-  if (pad != active_sinkpad)
-    goto not_active;

I don't think this makes sense, passthrough allocations should only be done on
the activated pad.

@@ +693,2 @@
     else
+      clock_time = GST_CLOCK_TIME_NONE;

You should put the change the uses the clock time for syncing into another
commit, separated from the queueing commit. Also the syncing to the clock
should be optional and the option to sync to the running time should be kept as
it's generally useful too

@@ +775,3 @@
+      /* the buffer is still valid if its duration is valid and the
+       * timestamp + duration is >= time, or if its duration is invalid
+       * and the timestamp is >= time */

What about buffers without valid timestamp?

@@ +832,3 @@
+      GstBuffer *old_buffer;
+
+      old_buffer = g_queue_pop_tail (selpad->buffers);

Pushing of all the old buffers should only happen for the first call to the
chain function after a stream switch. The size of the queue in the element
should also be configurable and have a maximum in sizes and time.

@@ +1311,3 @@
     /* no stop time given, get the latest running_time on the active pad to 
      * close and open the new segment */
+    stop_time = gst_selector_pad_get_running_time (old);

You get the stop time from the element's running time but the start time from
the clock. This could even cause start_time to be larger than stop_time

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