[Bug 722511] [RFC][PATCH] Introduce tq: tee element with embedded queue elements on srcpads

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #6 from Andrey Utkin <me at andrey-utkin.pp.ua> 2014-01-19 13:07:16 UTC ---
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Didn't review it yet but I think it might be beneficial to use IDLE probes
> > > here. Should be just an optimization though.
> > 
> > I don't think so.
> > If application developer follows manuals, then it is very probable that
> > gst_tq_release_pad() is executed in the inside queue's push thread. And a tee
> > pad callback does gst_element_set_state (queue, GST_STATE_NULL), which includes
> > joining queue's thread, which would deadlock in case we run it on same thread,
> > which is possible with IDLE probe.
> 
> Well, the block callbacks will also be called from the streaming thread. So you
> have the same problem there. You need to do the shutdown from another thread
> that is triggered from the probe callback.

Let's define contexts.
IDLE probe at application context for releasing tq pad is OK, we must work
correctly with any legal possible action from application.
Setup of IDLE probe in gst_tq_release_pad() to release tee pad and set queue to
NULL state is NOT OK, because IDLE may result on callback execution (including
...set_state (queue, ...NULL)) at same thread: if application executed
gst_element_release_request_pad (tq, pad) on queue push thread, this would
deadlock.

> > BTW What do you think about the following place in patch, does it serve right
> > purpose?
> > 
> > +  // Don't execute on any upstream-directed cases
> > +  // thus we avoid situation of joining queue's task from itself
> > +  if (info->type & (GST_PAD_PROBE_TYPE_DATA_UPSTREAM
> > +          | GST_PAD_PROBE_TYPE_QUERY_UPSTREAM))
> > +    return ret;
> 
> I don't understand why it's necessary but it looks like a hack to me right now.
> See what I wrote about the other thread.

I added this in my app when i have seen a message about joining queue thread
from itself caused by callback being triggered from gst_pad_peer_query() - some
upstream-directed query has taken place. After this change the issue haven't
reproduced. I hope my understanding of the matter (more exact to say
"intuition") is correct.

> > > And when releasing a "tee ! queue" branch you will need to drain it, e.g.
> > > EOS event or DRAIN query. Didn't see anything while looking fast over the code.
> > 
> > I understand how i could send EOS event on queue sinkpad. But i don't
> > understand  should i tap on queue srcpad and listen until EOS event exits out
> > of it... And how to do that if it is indeed required.
> > Regarding DRAIN query, haven't seen its usage at all yet. But found your own
> > posting around the internet: "For the DRAIN query, while this in theory is true
> > the problem with that is that most elements that actually need to be drained
> > don’t implement handling of the query but instead do the same on EOS."
> 
> Yes, but queue implements handling of the DRAIN query fortunately :) You have
> to intercept the query or the event on the srcpad of the queue, and then from
> there shutdown the queue (see above, trigger another thread!)

Ok, i understand that it is designed way to do. But it feels so much hassle to
add that, and in my app i haven't noticed any leaks or other issues because of
this. What is the risk of not doing so? And if risk is real, what should it
take to eliminate this risk systematically, so app developers wouldn't be
required to do it in this case?

> No, it's not properly integrated into the doc build system then. You need to
> add it to the Makefile.am, do the make update-docs step, and add things to the
> *-sections.txt and add it to the *.sgml file there. It's all not very
> straightforward but there's documentation about it somewhere :)

Seems this is required?

diff --git a/docs/plugins/Makefile.am b/docs/plugins/Makefile.am
index 8758cf3..282dd14 100644
--- a/docs/plugins/Makefile.am
+++ b/docs/plugins/Makefile.am
@@ -163,6 +163,7 @@ EXTRA_HFILES = \
        $(top_srcdir)/gst/sdp/gstsdpdemux.h \
        $(top_srcdir)/gst/speed/gstspeed.h \
        $(top_srcdir)/gst/stereo/gststereo.h \
+       $(top_srcdir)/gst/tq/tq.h \
        $(top_srcdir)/gst/videosignal/gstvideoanalyse.h \
        $(top_srcdir)/sys/directdraw/gstdirectdrawsink.h \
        $(top_srcdir)/sys/dvb/gstdvbsrc.h \


But i don't see in this file any mentions of "mpegts", for example. But in repo
there are files docs/plugins/inspect/plugin-mpegtsdemux.xml ,
docs/plugins/inspect/plugin-mpegtsmux.xml . I don't understand the system.

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