GNOME Bugzilla – Bug 770627
adaptivedemux: prevent to set source flag itself
Last modified: 2016-09-10 19:55:00 UTC
I ran some validate test cases with playbin3, then it failed when doing EOS action with below command. The reason was that there was two src elements in the Urisourcebin, Souphttpsrc and Dashdemux. So the Dashdemux got the event first and Souphttpsrc in front of DashDemux returned false due to the sinkpad of Dashdemux already received the event and returns GST_FLOW_EOS. It was not a problem with Playbin2, Adaptivedemux (Dashdemux) is in the decodebin2 and there's only a single child source element on the Playbin point of view. Test case: GST_VALIDATE_SCENARIOS_PATH=/home/wonchul/work/gst/gst-devtools/validate/data/scenarios: GST_GL_XINITTHREADS=1 GST_VALIDATE_OVERRIDE=/home/wonchul/gst-validate/gst-integration-testsuites/medias/defaults/online-streams-infos/dash/dash.exMPD_BIP_TC1.override DISPLAY=:0 GST_VALIDATE_SCENARIO=play_15s /home/wonchul/work/gst/gst-devtools/validate/tools/gst-validate-1.0-debug playbin3 uri=http://127.0.0.1:8079/defaults/exMPD_BIP_TC1/exMPD_BIP_TC1.mpd audio-sink='fakesink sync=true' video-sink='fakesink sync=true' --set-media-info "/home/wonchul/gst-validate/gst-integration-testsuites/medias/defaults/online-streams-infos/dash/dash.exMPD_BIP_TC1.stream_info"
Created attachment 334502 [details] [review] bin: don't set source flag on adaptive demuxer
Review of attachment 334502 [details] [review]: ::: gst/gstbin.c @@ +1194,3 @@ + GST_ELEMENT_METADATA_KLASS), "Demux") + && strstr (gst_element_class_get_metadata (bclass, + GST_ELEMENT_METADATA_KLASS), "Adaptive")) { That's not the right approach, GstBin should not need to know about any such things. This is something that would have to be solved in adaptivedemux by making sure to unset the SOURCE flag after adding elements, etc.
Created attachment 334503 [details] [review] bin: don't set source flag on adaptive demuxer
Comment on attachment 334503 [details] [review] bin: don't set source flag on adaptive demuxer Same as before, fix needs to be in adaptivedemux
Created attachment 334507 [details] [review] adaptivedemux: prevent to set source flag itself yes, it should be handled in adaptivedemux, I fixed it to unset source flag of uri_handler before adding it to adaptivedemux.
Comment on attachment 334507 [details] [review] adaptivedemux: prevent to set source flag itself Does this really help? I would assume that you have to set it *after* adding the source element, as adding it will cause the flag to be set AFAIU?
Created attachment 334509 [details] [review] adaptivedemux: prevent to set source flag itself It works, removed source_flag of source element(uri_handler) before adding it to demux, but yes it seems better to unset the flag of adaptivedemux element like you said after adding source element. (without touching source element flag) update patchset.
Review of attachment 334509 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +2543,3 @@ gst_bin_add (GST_BIN_CAST (stream->src), uri_handler); + GST_OBJECT_FLAG_UNSET (stream->src, GST_ELEMENT_FLAG_SOURCE); Doing it a few lines below for demux after adding stream->src seems more correct. stream->src is technically a source.
Created attachment 334569 [details] [review] adaptivedemux: prevent to set source flag itself yes, thanks for the review, I fixed it to unset source flag of demux, not a source
This is going to be racy between gst_bin_add and GST_OBJECT_FLAG_UNSET. As this can happen while in PLAYING there is a chance the user tries to do some operation and gets an unexpected behavior if it hits this window. I guess to properly solve this we need to have it done inside gst_bin_add, a variant that would allow suppressing some flags propagation. This is also done in camerabin IIRC, though I'm not sure if it is racy there as it is done during startup.
That's indeed a good point, thanks! So maybe the solution for now would indeed be to keep the flag unset on stream->stream before adding it as that can't be observed from the outside. Adding a gst_bin_add() variant for this seems a bit problematic. What would you call it, and it would also have to come with a vfunc :) A add_full() that takes a GstBinAddPropagateElementFlags? A GstBinAddFlags that contains a flag for propagating the SINK/SOURCE/PROVIDES_CLOCK/etc flags?
Created attachment 334585 [details] [review] adaptivedemux: prevent to set source flag itself So, unset the source flag on stream->src before adding demux.
Comment on attachment 334585 [details] [review] adaptivedemux: prevent to set source flag itself Seems good to go for the time being, what do you think Thiago? Additionally we should try to find good GstBin API for this issue and keep this bug open. This however should get a FIXME comment added that explains why and links to this bug.
(In reply to Sebastian Dröge (slomo) from comment #11) > That's indeed a good point, thanks! So maybe the solution for now would > indeed be to keep the flag unset on stream->stream before adding it as that > can't be observed from the outside. > > Adding a gst_bin_add() variant for this seems a bit problematic. What would > you call it, and it would also have to come with a vfunc :) A add_full() > that takes a GstBinAddPropagateElementFlags? A GstBinAddFlags that contains > a flag for propagating the SINK/SOURCE/PROVIDES_CLOCK/etc flags? I'd prefer the opposite to tell which ones to suppress. gst_bin_add_suppress_flags() but the vfunc thing makes this a bit more involved. Another option is to allow setting this as a property in GstBin. No matter how many sources/sinks we set to adatpvidedemux it should not be considered a source or a sink. So maybe we want a property to prevent it from propagating flags at every add. What do you think?
A property on the bin would seem useful, yes. A value and mask of flags that should not be handled automatically.
It seems to be better to use property in GstBin to prevent propagating flags
Question is mostly how the API would look like. Would you like to propose a patch?
Marking as blocker for now. Either we should get the GstBin part in, or the existing patch.
Created attachment 334621 [details] [review] bin: Add supress flags property
Created attachment 334622 [details] [review] adaptivedemux: prevent to propagate source flag to itself added the property 'supress-flags' on the bin to prevent propagation of element's specific flags when it added to the bin.
Thanks for working on this. The implementation looks correct, just wondering if we want to have a discoverable property (will appear at every bin's property list) or a less discoverable set/get pair for use only when writing bin-derived elements? I'm more in favor of the later.
I also prefer a setter/getter (or even a class variable ;) ) over a property, especially as you need more than a property to actually set those flags correctly.
Created attachment 334760 [details] [review] bin: Add setter and getter to supress element flags
Created attachment 334761 [details] [review] adaptivedemux: prevent to propagate source flag to itself Fixed it to add setter/getter for 'supress flags' to the bin instead of discoverable property.
Review of attachment 334760 [details] [review]: Just some minor stuff to fix. ::: gst/gstbin.c @@ +1412,3 @@ +void +gst_bin_set_supress_flags (GstBin * bin, GstElementFlags flags) Missing documentation, remember to add the Since marker. Same for the getter function. Will this be a 1.10 or 1.12 addition? @@ +1414,3 @@ +gst_bin_set_supress_flags (GstBin * bin, GstElementFlags flags) +{ + g_return_val_if_fail (GST_IS_BIN (bin), NULL); This should be a g_return_if_fail as the function has no return value. @@ +1420,3 @@ + GST_OBJECT_UNLOCK (bin); + + GST_DEBUG_OBJECT (bin, "set supress flags(%d) to bin '%s'", flags, flags are usually better read in hex format. ::: gst/gstbin.h @@ +211,3 @@ +/* set and get supress flags */ +void gst_bin_set_supress_flags (GstBin * bin, GstElementFlags flags); +GstElementFlags gst_bin_get_supress_flags (GstBin * bin); <bikeshed> I'd prefer: gst_bin_set/get_supressed_flags </bikeshed>
Created attachment 335151 [details] [review] adaptivedemux: prevent to propagate source flag to itself
Created attachment 335152 [details] [review] bin: Add setter and getter to supress element flags Added the documentation for setter and getter related with supressed flags, fixed the name supress_flag to supressed_flag. I'm not sure what Since marker should be, I just wrote it to 1.10 here. Thanks for the review!
Review of attachment 335152 [details] [review]: Looks good, some minor typos on the docs that can be fixed when pushing.
Review of attachment 335151 [details] [review]: Looks good
s/supress/suppress/ ?
Looks good to me too except the typos, Thiago go ahead with merging (and don't forget the typos) :)
commit 29c3b8b4d572a93bb7d0f374ffc0d2894abce3da Author: Thiago Santos <thiagossantos@gmail.com> Date: Sat Sep 10 11:59:11 2016 -0300 tests: gstbin: add tests for suppressed flags Some simple tests to make sure it keeps working commit 978b7ace5210f2a6a3204fd22c5699a5b487aa07 Author: Wonchul Lee <wonchul.lee@collabora.com> Date: Fri Sep 2 17:39:17 2016 +0900 bin: Add setter and getter to suppress element flags Suppress-flags is for preventing propagation of child element's specific flag when it is added to the bin. https://bugzilla.gnome.org/show_bug.cgi?id=770627 commit aa314c43baf37ed4ebf9ac359edb1784dd4e3bf4 Author: Wonchul Lee <wonchul.lee@collabora.com> Date: Mon Sep 5 10:31:40 2016 +0900 adaptivedemux: prevent to propagate source flag to itself https://bugzilla.gnome.org/show_bug.cgi?id=770627
Comment on attachment 335152 [details] [review] bin: Add setter and getter to supress element flags Committed with typo fixes.
This is breaking the build in Continuous. The GStreamer API in commit: https://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=978b7ace5210f2a6a3204fd22c5699a5b487aa07 has: gst_bin_get_suppressed_flags gst_bin_set_suppressed_flags but the gst-plugins-bad commit: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=aa314c43baf37ed4ebf9ac359edb1784dd4e3bf4 has: gst_bin_set_supressed_flags I'll tag gst-plugins-bad in Continuous in the meantime.
Sorry, my mistake. Made the changes locally and forgot to amend before pushing. Follow up fix in a min.
commit 08da51452879bc53dfaf1635598cf0b7184e4196 Author: Thiago Santos <thiagossantos@gmail.com> Date: Sat Sep 10 16:41:28 2016 -0300 adaptivedemux: fix typo in new API Fixes supressed -> suppressed typo in previous commit https://bugzilla.gnome.org/show_bug.cgi?id=770627 Done, thanks for noticing.