[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

--- Comment #48 from Andre Moreira Magalhaes <andrunko at gmail.com> 2012-05-21 17:55:22 UTC ---
(In reply to comment #47)
> Review of attachment 214509 [details]:
> 
> ::: 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
I forgot to change it when changing the event to always lock over the switch
statement, will change.

> @@ -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.
The issue is that we cannot return NOT_LINKED in buffer_alloc if we did not
push anything to the active pad yet as it was doing, will update here.

> @@ +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
So with the patch we only use the clock in "sync-streams" mode and without
using it it wouldn't work, should I really keep the old behaviour that does not
work as expected? I don't think we should and also no split should be required
in the patch if that is the case

> @@ +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?
buffers with invalid timestamps should be ignored always, I will update.

> @@ +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. 
This should be true already, will double check.

> The size of the queue in the element
> should also be configurable and have a maximum in sizes and time.
Will add it.

> @@ +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
The stop time is used for the CLOSED newsegment and the start time for the
UPDATE newsegment, so I don't think there is any issue here.

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