[Bug 709224] audio/videodecoder: Not returning GST_FLOW_EOS when after segment

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #35 from Thiago Sousa Santos <thiago.sousa.santos at collabora.co.uk> 2013-11-04 22:23:08 UTC ---
(In reply to comment #34)
> Generally looks fine to me (thanks for reworking this!), but I still have a
> nagging feeling that it could be even better.
> 
> Some random comments:
> 
>  - in the commit message: GstFlowReturnCombiner -> GstFlowCombiner
> 
>  - FLOW_FLUSH aggregation should be documented in docs chunk too
> 
>  - gst_flow_combiner_update_flow_return() - quite long, how about
>    just _add_flow() or _combine_flow()?

Agreed.

> 
>  - gst_flow_combiner_get_flow_return(): do we really need to iterate over
>    all pads every time? Can't we store the last aggregated flow return and
>    combine that with the new one in most cases?

Good idea, this would make the default GST_FLOW_OK case as fast as before with
tee and the demuxers.

> 
>  - all this hashtable stuff irks me a little, even if it's just an internal
>    implementation detail.
> 
>  - wonder if it shouldn't go into gstpad.c as GstPadFlowCombiner, then we
>    could just use the GstPad private struct and store the last flow there ;)
>    (or we could do that anyway via some internal helper funcs if we wanted to)

Would the GstFlowCombiner be setting it directly or pad_push would do it
automatically? If this is the case we could make this variable public directly
or via a function, as most demuxers seem to use it. Is this useful elsewhere?

This would get rid of the hashtable and we could use a linked list instead.

> 
>  - another idea: I think most elements (esp. demuxers) have a derived pad or
>    pad-related stream structure which they operate on, so they could store
>    some kind of GstFlowCombinerPadID in there as well. You could hand those
>    out when the demuxer/element does _add_pad(), and it could just be a
>    counter or whatever (just typedef it to guintptr then it can be anything
>    in future), then you don't need the hashtable and can just do pads[i]
>    inside the flow combiner, etc.

This is a good idea, too. But what happens when a pad is removed? We can just
set it to NOT_LINKED and everything else should work, but then the array of
pads
will only increase and we can have the flow iteration taking longer and longer,
aside from memory consumption for the arrays (unless we iterate the array or
have some mechanism to reuse old ids when new pads are added).

The 1st idea is simpler to implement, but the 2nd is less invasive. Going to
try the 2nd now.

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