GNOME Bugzilla – Bug 748233
GstObject: add support for setting uniquely numbered names for simplicity, e.g. videoqueue-%u
Last modified: 2018-11-03 12:27:07 UTC
Created attachment 302051 [details] [review] gstobject: Add support for a format-specifier suffix Similar to how the default name for an element is automatically assigned to be unique, it would be useful to be able to pass a custom name that can be automatically made unique. For instance, if I have a program with a complex pipeline with queues in several different contexts (after decoders, after tees, etc), I want to be able to specify a context-specific name such as "video-decoder-queue-%u", "tee-queue-%u", etc which will become "video-decoder-queue-0", "video-decoder-queue-1", and so on. This makes parsing debug logs much easier when you see "video-decoder-queue-12" instead of "queue34". The attached patch implements this. gst_element_factory_make (queue, "somequeue-%u"); will create "some-queue-0", "some-queue-1", etc using the same infrastructure as the code that does the default name generation.
Comment on attachment 302051 [details] [review] gstobject: Add support for a format-specifier suffix I think this is a nice idea and would be useful to have. Two comments: - gst_object_set_name_numbered() and _set_name_default() share 90% of the code. Perhaps that datalist thing could be refactored into a shared internal utility function (pass quark + ..) ? - is there a reason to mandate the hyphen in the suffix rather than just check for %u ?
(In reply to Tim-Philipp Müller from comment #1) > Comment on attachment 302051 [details] [review] [review] > gstobject: Add support for a format-specifier suffix > > I think this is a nice idea and would be useful to have. > > Two comments: > > - gst_object_set_name_numbered() and _set_name_default() > share 90% of the code. Perhaps that datalist thing > could be refactored into a shared internal utility > function (pass quark + ..) ? > Yep, my first attempt was by adding branches in _set_name_default(), and it was quite ugly. I'll refactor out the datalist stuff so it can be reused. Didn't think of that. :) > - is there a reason to mandate the hyphen in the suffix > rather than just check for %u ? So, in _set_name_default(), there's this code: /* give the 20th "queue" element and the first "queue2" different names */ l = strlen (type_name); if (l > 0 && g_ascii_isdigit (type_name[l - 1])) { name = g_strdup_printf ("%s-%d", type_name, count); } else { name = g_strdup_printf ("%s%d", type_name, count); } I wanted to avoid the same problem here, so it made sense to mandate the suffix to be "-%u" rather than duplicate that hack here. In my opinion, for consistency, even that code should always just do "-%d" instead of checking the last character in the type_name.
Created attachment 302074 [details] [review] gstobject: Add support for a format-specifier suffix The datalist manipulation is now in a shared inline function.
Created attachment 302127 [details] [review] gstobject: Add support for a format-specifier suffix Fixed an off-by-one error while removing the format-specifier suffix
A simple unit test that exercises this new code path would be nice IMO. This suffix_index = (int) (g_strrstr (name, "%u") - name) - 1; could probably be simplified based on strlen() and the previous has_suffix() check?
Created attachment 304067 [details] [review] gstobject: Add support for a format-specifier suffix (In reply to Tim-Philipp Müller from comment #5) > A simple unit test that exercises this new code path would be nice IMO. > Added. > This > > suffix_index = (int) (g_strrstr (name, "%u") - name) - 1; > > could probably be simplified based on strlen() and the previous has_suffix() > check? Err, yes, you're right. The index is simply strlen(name) - strlen("-%u"). Fixed. :)
I think there is one problem with this: GstPadTemplates are GstObjects as well and actually set their name to things like "src_%u" and I don't think we'd want that replaced with "src_9871".
Ah, but we replace "-%u" with "-432", etc, so "_%u" should be left as-is, right?
For request pads, also see bug #761225 which makes this even more tricky :) The request pad part seems like a real problem, meaning that we might not be able to solve this easily for 1.x.
-- 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/gstreamer/issues/108.