GNOME Bugzilla – Bug 722980
encodebin: should defer setting up profiles to state>NULL
Last modified: 2018-11-03 11:28:15 UTC
If one creates and encodebin and does a g_object_set(encbin, "profile", profile, NULL); encodebin would immediately try to activate the profile (see gst_encode_bin_set_profile()). If a profile cannot be setup, encodebin would emit missing plugin messages. Now at this point it is not yet plugged in a pipeline and thus the messages are send to void. Should we document this or defer activating the profile to state>NULL?
I think it should be just done in READY->NULL. Doesn't really make sense otherwise.
You mean NULL->READY, right?
Yes
Yieks, we can't do this: gst_element_link_many (audiotestsrc, ebin, fakesink, NULL) as we don't have pads yet. Just having an identify element there by default is probably not good enough to fix it. That leaves us with option 2: documenting that if one wants to get the missing-plugin message, either add a bus to the element and listen or first add the element, set profile and later link.
Created attachment 269333 [details] [review] encodebin: activate profiles when going from NULL->READY
(In reply to comment #4) > Yieks, we can't do this: > > gst_element_link_many (audiotestsrc, ebin, fakesink, NULL) > > as we don't have pads yet. Just having an identify element there by default is > probably not good enough to fix it. > > That leaves us with option 2: documenting that if one wants to get the > missing-plugin message, either add a bus to the element and listen or first add > the element, set profile and later link. Isn't that expected? That's just like with decodebin and the sometimes pads really. You add all elements you want to a bin, then connect everything whenever possible.
Comment on attachment 269333 [details] [review] encodebin: activate profiles when going from NULL->READY I think you should move the tear_down() bit to READY->NULL state change for symmetry. Otherwise this makes sense.
(In reply to comment #6) > (In reply to comment #4) > > Yieks, we can't do this: > > > > gst_element_link_many (audiotestsrc, ebin, fakesink, NULL) > > > > as we don't have pads yet. Just having an identify element there by default is > > probably not good enough to fix it. > > > > That leaves us with option 2: documenting that if one wants to get the > > missing-plugin message, either add a bus to the element and listen or first add > > the element, set profile and later link. > > Isn't that expected? That's just like with decodebin and the sometimes pads > really. You add all elements you want to a bin, then connect everything > whenever possible. I agree that it would be expected, but given how many tests it breaks I wonder how many applications this breaks.
Created attachment 269609 [details] [review] encodebin: activate profiles when going from NULL->READY Instead of immediately activating a profile when it was set, just store it and activate it when we go to READY. This allows to create an encodebin, set the profile and add it to the pipeline, while only checking the pipeline bus for missing_plugin messages.
I don't think changing the API to delay the creation of ther pads to NULL->READY is acceptable in 1.x, will break too many applications. One thing you could do is to create just the pads on g_object_set(), although this will be problematic if the number of pads is dynamic (0).
Just creating the pads would be quite ugly. I'd rather document the issue for now and include it when we decide to start encodebin2. I'd prefer the already submit the test parts though, as this is harmless. It people copy'n'paste code from tests, they'll do this right from the start.
Just to report - so far I tested the change with buzztrax and transmageddon. It seems that the change mostly breaks smaller tests. Real apps need to handle pads dynamically anyway. E.g.: https://git.gnome.org/browse/transmageddon/tree/src/transcoder_engine.py#n176
-- 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-base/issues/105.