GNOME Bugzilla – Bug 722511
tq: tee element with embedded queue elements on srcpads
Last modified: 2018-11-03 13:20:03 UTC
Created attachment 266633 [details] [review] patch This was discussed in gstreamer-devel maillist a bit, and i was encouraged to post this as a patch to -plugins-bad. Besides inclusion into upstream, what interests me is code review, so i figure out if used things are operated correctly. Any comments appreciated. The code currently lacks management of inside tee and queues properties. I haven't decided yet how exactly they would be passed through, possibly as two properties "tee-props" and "queue-props" as strings representing a set of key-value pairs. P. S. How xml doc is generated?
I think the Pexip folks (hgr) have one like this as well.
Didn't review it yet but I think it might be beneficial to use IDLE probes here. Should be just an optimization though. Also don't use // comments but /* */ :) 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. (In reply to comment #0) > The code currently lacks management of inside tee and queues properties. I > haven't decided yet how exactly they would be passed through, possibly as two > properties "tee-props" and "queue-props" as strings representing a set of > key-value pairs. See the GstChildProxy interface. For the queues it might be good to have default settings for newly created queues as part of the "tq" element though. I wouldn't recommend using a generic string property with key-value pairs. Add a proper property for everything you need. > P. S. How xml doc is generated? You add stuff to docs/plugins/Makefile.am and then run "make update-docs". Or something like that :)
(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. 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; > Also don't use // comments but /* Oh damn. Ok, will do. > 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." > (In reply to comment #0) > > > The code currently lacks management of inside tee and queues properties. I > > haven't decided yet how exactly they would be passed through, possibly as two > > properties "tee-props" and "queue-props" as strings representing a set of > > key-value pairs. > > See the GstChildProxy interface. For the queues it might be good to have > default settings for newly created queues as part of the "tq" element though. Does it mean that no special code is required (because GstTq is a subclass of GstBin), and application can listen for signals "child-added", "child-removed" and set desired properties in the signal callback? > > P. S. How xml doc is generated? > > You add stuff to docs/plugins/Makefile.am and then run "make update-docs". Or > something like that :) Thanks. Is that right that GstTq currently does not need to generate any help, because it does not provide any special API?
(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. > 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. > > 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!) > > See the GstChildProxy interface. For the queues it might be good to have > > default settings for newly created queues as part of the "tq" element though. > > Does it mean that no special code is required (because GstTq is a subclass of > GstBin), and application can listen for signals "child-added", "child-removed" > and set desired properties in the signal callback? Yes, might not be the most beautiful API but that's what automatically works already :) > > > P. S. How xml doc is generated? > > > > You add stuff to docs/plugins/Makefile.am and then run "make update-docs". Or > > something like that :) > > Thanks. > Is that right that GstTq currently does not need to generate any help, because > it does not provide any special API? 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 :)
Actually with the DRAIN query you don't need another thread. You would block (normal block or idle) the tee srcpad. Install a query probe on the queue srcpad. Send a DRAIN query, drop the DRAIN query from the query probe. Then from the block callback shut down the queue.
(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.
Seems update-docs target does not exist anywhere.
It's "make update", sorry. The risk of not draining the queues is data loss and still data flow happening while you release the pads (because queue might still be pushing data). The problem of not using IDLE probes is that you might never release a pad then if there's no further data flow. That's not very expected from the application I guess. This dropping of upstream data resulting in shutdown from the streaming thread seems like an indication that something is not yet as it should be ;) Do you have a backtrace of this?
(In reply to comment #8) > The problem of not using IDLE probes is that you might never release a pad then > if there's no further data flow. That's not very expected from the application > I guess. Using IDLE probe does not guarantee against callback being never executed, too. That's why i proposed guaranteed-synchronous probe type. Maybe that's not that bad idea to have it? > This dropping of upstream data resulting in shutdown from the streaming thread > seems like an indication that something is not yet as it should be ;) Do you > have a backtrace of this? You must be talking about + if (info->type & (GST_PAD_PROBE_TYPE_DATA_UPSTREAM + | GST_PAD_PROBE_TYPE_QUERY_UPSTREAM)) No, i haven't saved a backtrace and haven't got a testcase for it. I remember it to happen surely only once. I don't remember what exactly the query was, and what was the pipeline. Maybe you could make a case to trigger this scenario, with your knowledge.
(In reply to comment #9) > (In reply to comment #8) > > The problem of not using IDLE probes is that you might never release a pad then > > if there's no further data flow. That's not very expected from the application > > I guess. > > Using IDLE probe does not guarantee against callback being never executed, too. > That's why i proposed guaranteed-synchronous probe type. Maybe that's not that > bad idea to have it? True, if the pad is already deadlocked it will not be called, or if it is PAUSED forever :) Otherwise the IDLE probe will be called even if there never ever is any new data, while the BLOCK probes need actual data flow. > > This dropping of upstream data resulting in shutdown from the streaming thread > > seems like an indication that something is not yet as it should be ;) Do you > > have a backtrace of this? > > You must be talking about > + if (info->type & (GST_PAD_PROBE_TYPE_DATA_UPSTREAM > + | GST_PAD_PROBE_TYPE_QUERY_UPSTREAM)) > > No, i haven't saved a backtrace and haven't got a testcase for it. I remember > it to happen surely only once. I don't remember what exactly the query was, and > what was the pipeline. Maybe you could make a case to trigger this scenario, > with your knowledge. Well, maybe your block probe was called because of the upstream QoS event... and then you removed the tee branch from there... i.e. the streaming thread of the queue in the worst case.
(In reply to comment #10) > True, if the pad is already deadlocked it will not be called, or if it is > PAUSED forever :) > > Otherwise the IDLE probe will be called even if there never ever is any new > data, while the BLOCK probes need actual data flow. Doesn't IDLE probe falls back to asynchronous calling if the pad is blocked at the moment of add_probe()? If so, then in general case IDLE is nothing better than BLOCK, except having even more indeterminity of actual thread executing callback procedure. > Well, maybe your block probe was called because of the upstream QoS event... > and then you removed the tee branch from there... i.e. the streaming thread of > the queue in the worst case. So, on higher level of view, does that "if" actually helps GstTq to work the right way in general case? Would it be right if, before releasing tq pad, application blocks a sinkpad connected to tq so that nothing passes?
(In reply to comment #11) > (In reply to comment #10) > > True, if the pad is already deadlocked it will not be called, or if it is > > PAUSED forever :) > > > > Otherwise the IDLE probe will be called even if there never ever is any new > > data, while the BLOCK probes need actual data flow. > > Doesn't IDLE probe falls back to asynchronous calling if the pad is blocked at > the moment of add_probe()? If so, then in general case IDLE is nothing better > than BLOCK, except having even more indeterminity of actual thread executing > callback procedure. Yes, but it will be called if there is never ever any data flow anymore on the pad. A BLOCK not. > > Well, maybe your block probe was called because of the upstream QoS event... > > and then you removed the tee branch from there... i.e. the streaming thread of > > the queue in the worst case. > > So, on higher level of view, does that "if" actually helps GstTq to work the > right way in general case? > > Would it be right if, before releasing tq pad, application blocks a sinkpad > connected to tq so that nothing passes? No, the application should not need to worry about anything and just call gst_element_release_pad(). And internally you care for all that.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > True, if the pad is already deadlocked it will not be called, or if it is > > > PAUSED forever :) > > > > > > Otherwise the IDLE probe will be called even if there never ever is any new > > > data, while the BLOCK probes need actual data flow. > > > > Doesn't IDLE probe falls back to asynchronous calling if the pad is blocked at > > the moment of add_probe()? If so, then in general case IDLE is nothing better > > than BLOCK, except having even more indeterminity of actual thread executing > > callback procedure. > > Yes, but it will be called if there is never ever any data flow anymore on the > pad. A BLOCK not. What about a situation that direct synchronous execution is not performed because the pad is blocked by some data flow, and asynchronous execution does not take place also because that data flow act which prevented synchronous execution was LAST on that pad? > > > Well, maybe your block probe was called because of the upstream QoS event... > > > and then you removed the tee branch from there... i.e. the streaming thread of > > > the queue in the worst case. > > > > So, on higher level of view, does that "if" actually helps GstTq to work the > > right way in general case? > > > > Would it be right if, before releasing tq pad, application blocks a sinkpad > > connected to tq so that nothing passes? > > No, the application should not need to worry about anything and just call > gst_element_release_pad(). And internally you care for all that. Well, the context was that this "if" is internal to GstTq, but not a something that should be added in application code. And the reason it is added is to avoid joining queue thread from itself.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #10) > > > > True, if the pad is already deadlocked it will not be called, or if it is > > > > PAUSED forever :) > > > > > > > > Otherwise the IDLE probe will be called even if there never ever is any new > > > > data, while the BLOCK probes need actual data flow. > > > > > > Doesn't IDLE probe falls back to asynchronous calling if the pad is blocked at > > > the moment of add_probe()? If so, then in general case IDLE is nothing better > > > than BLOCK, except having even more indeterminity of actual thread executing > > > callback procedure. > > > > Yes, but it will be called if there is never ever any data flow anymore on the > > pad. A BLOCK not. > > What about a situation that direct synchronous execution is not performed > because the pad is blocked by some data flow, and asynchronous execution does > not take place also because that data flow act which prevented synchronous > execution was LAST on that pad? Well, that would mean it would be blocked forever in the sink for some reason (like PAUSED forever or a deadlock). If it was the last buffer, at some point gst_pad_push() will return... and then the IDLE probe will be called. > > > > Well, maybe your block probe was called because of the upstream QoS event... > > > > and then you removed the tee branch from there... i.e. the streaming thread of > > > > the queue in the worst case. > > > > > > So, on higher level of view, does that "if" actually helps GstTq to work the > > > right way in general case? > > > > > > Would it be right if, before releasing tq pad, application blocks a sinkpad > > > connected to tq so that nothing passes? > > > > No, the application should not need to worry about anything and just call > > gst_element_release_pad(). And internally you care for all that. > > Well, the context was that this "if" is internal to GstTq, but not a something > that should be added in application code. And the reason it is added is to > avoid joining queue thread from itself. Yeah, can you get a backtrace for the joining thread from itself problem? It indicates a conceptual problem elsewhere imo
(In reply to comment #14) > > What about a situation that direct synchronous execution is not performed > > because the pad is blocked by some data flow, and asynchronous execution does > > not take place also because that data flow act which prevented synchronous > > execution was LAST on that pad? > > Well, that would mean it would be blocked forever in the sink for some reason > (like PAUSED forever or a deadlock). If it was the last buffer, at some point > gst_pad_push() will return... and then the IDLE probe will be called. Good if so, it's just not described anywhere. > > > > > Well, maybe your block probe was called because of the upstream QoS event... > > > > > and then you removed the tee branch from there... i.e. the streaming thread of > > > > > the queue in the worst case. > > > > > > > > So, on higher level of view, does that "if" actually helps GstTq to work the > > > > right way in general case? > > > > > > > > Would it be right if, before releasing tq pad, application blocks a sinkpad > > > > connected to tq so that nothing passes? > > > > > > No, the application should not need to worry about anything and just call > > > gst_element_release_pad(). And internally you care for all that. > > > > Well, the context was that this "if" is internal to GstTq, but not a something > > that should be added in application code. And the reason it is added is to > > avoid joining queue thread from itself. > > Yeah, can you get a backtrace for the joining thread from itself problem? It > indicates a conceptual problem elsewhere imo Assume the application wants to release a requested pad of tq. It is done my setting up a probe callback that does gst_element_release_request_pad (tq, pad) and returns ...REMOVE. The most probable thread on which this callback could execute, if all is linked and playing, is the pushing thread of queue behind the subject tq pad. So in gst_tq_release_pad(), we have to account this, and avoid doing ...set_state (queue, ...NULL) straight in the procedure. Instead, what is currently implemented is doing it from internal tee's srcpad probe. I think there's no conceptual problem at this point. But, as you have confirmed my observation, in this environment the callback can be executed by upstream-directed flow, exactly the tee's srcpad probe callback in this case. And this can result in tee srcpad probe callback being executed by the queue thread, and doing ...set_state (queue, ...NULL) will deadlock or at last give a critical assertion warning. I haven't found a possibility to exclude upstream-directed cases by bitmask passed to ...add_probe(), so i have excluded these cases as "if" in the procedure itself.
Sebastian, could you please comment on my explanation of tq pad releasing procedure complexity?
(In reply to comment #15) > Assume the application wants to release a requested pad of tq. > It is done my setting up a probe callback that does > gst_element_release_request_pad (tq, pad) and returns ...REMOVE. A probe on which pad? When the application wants to remove the request pad of tq, you could just: 1) set up a block (data-downstream) on the tee srcpad 2) from there set up a query probe on the queue srcpad 3) do the drain query and drop it in the query probe 4) release the tee srcpad, shutdown the queue Adding the IDLE probe to the mix (for 1)) will require you to decouple 3/4 but doing them in another thread, and from that thread you would remove the probe created in 1. Does that make sense? > The most probable thread on which this callback could execute, if all is linked > and playing, is the pushing thread of queue behind the subject tq pad. It's going to be the streaming thread upstream of the tee... or for upstream events like QoS (on which you should not block really!) the streaming thread of the queue srcpad. > So in gst_tq_release_pad(), we have to account this, and avoid doing > ...set_state (queue, ...NULL) straight in the procedure. Instead, what is > currently implemented is doing it from internal tee's srcpad probe. > I think there's no conceptual problem at this point. > But, as you have confirmed my observation, in this environment the callback can > be executed by upstream-directed flow, exactly the tee's srcpad probe callback > in this case. And this can result in tee srcpad probe callback being executed > by the queue thread, and doing ...set_state (queue, ...NULL) will deadlock or > at last give a critical assertion warning. > > I haven't found a possibility to exclude upstream-directed cases by bitmask > passed to ...add_probe(), so i have excluded these cases as "if" in the > procedure itself. Try with GST_PAD_PROBE_TYPE_BLOCK_DOWNSTREAM
(In reply to comment #17) > (In reply to comment #15) > > > Assume the application wants to release a requested pad of tq. > > It is done my setting up a probe callback that does > > gst_element_release_request_pad (tq, pad) and returns ...REMOVE. > > A probe on which pad? When the application wants to remove the request pad of > tq, you could just: > > 1) set up a block (data-downstream) on the tee srcpad > 2) from there set up a query probe on the queue srcpad > 3) do the drain query and drop it in the query probe > 4) release the tee srcpad, shutdown the queue > > Adding the IDLE probe to the mix (for 1)) will require you to decouple 3/4 but > doing them in another thread, and from that thread you would remove the probe > created in 1. > > Does that make sense? No, because tee srcpad is inside tq and is unavailable at application context. And i don't like the idea that application needs to find internal tee, and manage probes on its pads, and manage queue elements. So, we want tq to behave in opaque way and be managable via gst_element_release_request_pad(). For this, the pad releasing operation can only start with blocking probe on tq srcpad and executing gst_element_release_request_pad(tq, pad) from this probe callback, then following a procedure i described in comment #15 and which is currently implemented in a patch.
You misunderstood me, sorry. gst_element_release_request_pad(tq, srcpad) → gst_tq_release_request_pad(tq, srcpad) → gst_pad_add_probe(tee_srcpad, ...) So the application only knows about tq and its srcpads. And you internally do that.
(In reply to comment #19) > You misunderstood me, sorry. > > gst_element_release_request_pad(tq, srcpad) → > gst_tq_release_request_pad(tq, srcpad) → > gst_pad_add_probe(tee_srcpad, ...) > > So the application only knows about tq and its srcpads. And you internally do > that. Ok. I think this discussion becomes so long and hard to follow that we forget the initial subject. So i'd like to start from the beginning. Sebastian, with your vision of my current implementation (except that "if" filtering out upstream events at tee srcpad probe), do you agree that it is possible that disposing queue is executed from queue thread? In which way could this be fixed? Sorry if you have already answered this question, but i just don't see it. If you don't like to continue this hard discussion, please feel free to answer just with a patch over my patch.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/126.