[Bug 638168] textoverlay: don't return wrong-state when stopping while in text chain
GStreamer (bugzilla.gnome.org)
bugzilla at gnome.org
Mon May 21 10:55:29 PDT 2012
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.
More information about the gstreamer-bugs
mailing list