GNOME Bugzilla – Bug 329069
API: add gst_parse_bin_from_description
Last modified: 2006-02-04 15:04:04 UTC
gst_bin_parse_bin_from_description() + unlinking unconnected ghost pads is code that is used and duplicated all over the place (gconf plugin, gnome-sound-recorder, sound-juicer, ...), so it seems it would make sense to put this as a utility function into the core. gst_bin_find_unconnected_pad() is needed by gst_bin_parse_bin_from_description(), and seems a useful utility function to expose. Patch to add both gst_parse_bin_from_description() and gst_bin_find_unconnected_pad() attached.
Created attachment 58317 [details] [review] adds gst_bin_parse_from_description() and gst_bin_find_unconnected_pad()
erm, _linking_ unconnected ghost pads I meant of course.
gst_bin_find_unconnected_pad has a locking order of parent -> child, which is not the case. You should iterators to avoid locking order problems.
Well Wim claims that the locking order isn't really settled -- to me that means that to avoid problems one should if possible avoid taking more than one object lock at a time.
Created attachment 58454 [details] [review] updated patch, using iterators Updated patch, this time using iterators. The gst_bin_find_unconnected_pad() function is primarily meant for newly-created elements that are in NULL state, so I'm not sure how important locking order is here in practice (we could just document it and worry about the other cases when needed). So, can I use iterators like this? Or do I have to create a list of elements first and then traverse that list and iterate their pads?
Well, it's probably best when adding new API to ensure there aren't any gotchas. Hence the iterators. I think your patch is correct, but it's tough to tell without a test. I prefer to use gst_iterator_fold, but that's me. So for me, I would say go ahead and commit -- but a short test would be most excellent.
Not entirely sure what exactly a test case would be supposed to test here - did you have anything specific in mind? Just create a bin with ten identities in it and see if it successfully ghostpads the loose ends and prerolls when put in between a fakesrc and fakesink? gconf*{sink|src} would be testing the functionality of these functions as well.
Committed to CVS, keeping bug open for the test case.
To me a test would take a data structure like this: { { 'identity ! fakesink', '/identity0/sink' }, { 'identity ! fakesink identity ! fakesink0.', '/identity0/sink,/identity1/sink' }, { NULL, NULL } } Where the first member is a pipeline description, and the second is a comma-separated list of pad names or so. The test would then run parse_bin_from_description on the description, and then check that the ghost pad targets are correct. I wouldn't bother with testing dataflow, that is tested elsewhere.
The current code only creates a max. of one ghost pad per direction though (which I've mentioned in the documentation). Not sure it should be doing more because of pad names and such - it's only a convenience function after all. More complex stuff the caller has to do himself IMHO.
Sure then, makes it simpler -- but I would have basic tests for this one. Same structure, just one pad path.
Tests added: 2006-02-04 Tim-Philipp Müller <tim at centricular dot net> * tests/check/gst/gstutils.c: (test_parse_bin_from_description), (gst_utils_suite): Add some simple tests for gst_parse_bin_from_description() and gst_bin_find_unconnected_pad() (#329069).
Excellent!