[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

Andre Moreira Magalhaes <andrunko> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #214509|0                           |1
        is obsolete|                            |

--- Comment #49 from Andre Moreira Magalhaes <andrunko at gmail.com> 2012-05-22 04:23:04 UTC ---
Created an attachment (id=214630)
 --> (https://bugzilla.gnome.org/attachment.cgi?id=214630)
gstinputselector: Properly sync when changing streams.

Here goes another round. I fixed some issues with the patch and also made some
improvements. Comments bellow.

(In reply to comment #48)
> (In reply to comment #47)
> > Review of attachment 214509 [details] [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.
Actually, after a new check active_selpad was never set before in this method,
only active_sinkpad, that is why we are retrieving it there.

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

> > @@ +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
Made some changes on how this works now. Also kept full-compatibility with the
old non "sync-streams" behaviour.

> > @@ +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.
Fixed.

> > @@ +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.
Actually it was not, but now it is :).

> > The size of the queue in the element
> > should also be configurable and have a maximum in sizes and time.
> Will add it.
Not yet. Do we really need this even with the cached buffers being discarded
when old? I don't think we should as this may break the synchronization, but if
so what should we use as defaults here? 

> > @@ +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.
No change here anymore for non "sync-streams" mode, using a different method
when in "sync-streams" mode.

The patch is a big improvement over the last one, as it is less intrusive,
keeping full compatibility with old behaviour for non "sync-streams" mode, and
it guarantees that the proper buffer newsegments are sent when sending cached
buffers.

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