GNOME Bugzilla – Bug 761225
pad: Allow multiple conversion specifiers for request pads and don't require it at the very end
Last modified: 2016-11-01 18:30:32 UTC
It's not a bug report and I just want to talk about the conversion specification on request padtemplate. It's about the request padtemplate has been allowed only one conversion, e.g. "src_%u". let's assume that there's a bin contains several tee elements. Then I want to pick specific tee element and request srcpad at one go, so thought about the "src_%u_%u" type of padtemplate for it. I'm not sure how should it pick the tee element in the bin automatically, maybe we can request "src_1_%u" (1 = element number), or somehow nicely. Of course, you may not consider using multiple conversion without wrapping such an element, tee, but if it does make sense, it could be useful to construct a new dynamic pipeline.
Not sure I understand what you mean here. Elements like rtpbin already have pad templates like recv_rtp_src_%u_%u_%u. It's the job of the element to make sense of these strings and produce a name that is compatible with the template name. So in your example your bin subclass would do that in GstElement::request_new_pad() and based on the name given by the application choose the appropriate internal tee element.
yes, but the rtpbin pad template "recv_rtp_src_%u_%u_%u" has a sometimes presence and it has been created inside of the element. does it fine to create sometimes pad using request_pad api? If not, I should have said it's about the request presence.
No, but you should be able to create a request pad template with that name. Is it not possible?
Apparently it's not allowed by name_is_valid() from gstpadtemplate.c. I think it would make sense to make these restrictions less strong. Not sure why they are there.
Reason is probably the automatic pad requesting code when linking elements and e.g. _gst_element_request_pad() and gst_element_get_request_pad(). Those have to be updated too then.
yeah, If it's acceptable, I'm willing to write a patch to make the restriction less strong :)
Well, you'll have to check all the code that currently does assumptions about this and check if it can be changed.
Maybe the checks are correct, but need to be part of overridable code ?
There's nothing really overridable here. It's code for automatically selecting the appropriate request pad template when linking elements together.
I see, so upon request it needs to figure-out the right template. I might be mistaken, but is this a limitation of _get_request_pad() ? Because in that case I'd be fine in saying that for this case we should use the _request_pad() method.
Ideally it would be changed to work with both variants, but yes that would also be an option.
Created attachment 320410 [details] [review] [patch] Allow multiple conversion specifiers for request pads attached a patch for allowing multiple conversion specifiers for request pads, but it seems to be required a restriction. If the name of the pad is requested as "src_%u%u", there's no way to distinguish two specifiers after the pad is created. So, I added the restriction to use an underscore between specifiers.
That seems like a reasonable new restriction, especially as it's strictly more weak than the previous restrictions we had (i.e. does not forbid anything that worked before).
Review of attachment 320410 [details] [review]: Can you also add some more unit tests for this? For using things like gst_element_link/_pads() and gst_parse_launch() correct behaviour.
Created attachment 320688 [details] [review] [patch] Allow multiple conversion specifiers for request pads 2 yeah, I added simple tests to check gst_element_link/link_pads and gst_parse_launch API working with this.
Review of attachment 320688 [details] [review]: ::: gst/gstelement.c @@ +940,3 @@ data = name + (str - templ->name_template); + /* If it has multiple convention specifiers, check them all. Each convention -> conversion @@ +950,3 @@ + } else { + /* Can either be %s or %d or %u, do sanity checking for %d */ + if (*(str + 1) == 'd') { I think you can read after the string's memory here. Why would str+1 be a valid pointer? *str might be '\0'. Please also check all the other string parsing another time
Created attachment 324938 [details] [review] [patch] Allow multiple conversion specifiers for request pads 3 str is assigned by strchr (templ->name_template, '%') ,so it can point to '%' or NULL here and *(str+1) should be one of the characters ('s', 'd', 'u') otherwise request padtemplate will not be created.(The name_is_valid function in padtemplate guarantee that str+1 should be valid)
and I fixed convention -> conversion :)
Review of attachment 324938 [details] [review]: ::: gst/gstelement.c @@ -1028,3 @@ - templ->name_template); - /* see if we find an exact match */ - if (strcmp (name, templ->name_template) == 0) { Why did you remove the exact match case? @@ -1035,3 @@ - /* Because of sanity checks in gst_pad_template_new(), we know that %s - and %d and %u, occurring at the end of the name_template, are the only - possibilities. */ The comment would be good to take over and update accordingly to the new situation @@ +1078,3 @@ + /* Because of sanity checks in gst_pad_template_new(), we know that %s + and %d and %u, occurring at the end of the name_template, are the + only possibilities. */ That's not true anymore, right? There could be multiple, and they might also be in the middle not only at the end. E.g. src_%u_bla would also be valid @@ +1114,3 @@ + templ_found = TRUE; + break; + } else if (! !str ^ ! !data) { Instead of this with the *bitwise* XOR, check the separate cases here ::: gst/gstpadtemplate.c @@ +256,2 @@ } + if ((str = g_strrstr (name, "%")) && (*(str + 2) != '\0')) { You use strstr() elsewhere and g_strstr() here. Why? ::: tests/check/gst/gstelement.c @@ +442,3 @@ + + gst_element_class_add_pad_template (element_class, + gst_pad_template_new ("src_%u_%u_%u", GST_PAD_SRC, GST_PAD_REQUEST, What about templates with %d (and checking for negative! numbers)? And templates with multiple %s? And templates like src_%u_something? I think those should also be checked
Created attachment 327128 [details] [review] allow multiple conversion specifiers for request pad 4 the previous patch didn't allow template name like src_%u_something. I change it to accept something + %u or %u + something and add is_valid_template_name_of_request_pad function, %s,%d test cases also.
Created attachment 330175 [details] [review] allow multiple conversion specifiers for request pad 5 updated the patchset and fixed positive false case of the request padtemplate (in case of src%u%u) and rebase it to up-to-date master branch.
Created attachment 330977 [details] [review] allow multiple conversion specifiers for request pad ping
Review of attachment 330977 [details] [review]: Generally looks good but I wonder how we would decide whether "src_a_b_c" is for a template "src_%s", "src_%s_%s", "src%s", "src_%s%s", etc.? It seems to add ambiguity for %s templates, which was not there before. AFAIU we require a '_' after template parts ("%s", "%u", "%d", etc), but '_' is still a valid part of a string by itself.
Yes, it's very ambiguous if we regard '_' as a valid string for the request template, in that case, "src_a_b_c" seems to be impossible to find proper template. It can be the various template as you mentioned(src_%s, src_%s_%s ...). If we wrap the specifier with another special character to distinguish between multiple specifiers that would solve this ambiguous, template should be "src_(%s)_(%s)", looks little strange though. We've just used '_' to specify strings conventionally and thought that "src_a_b_c" is the "src_%s_%s_%s" instinctively. So I suggest not to allow '_' as a part of request template name.
How should forbidding '_' as part of %s be done in a backwards compatible way?
It seems that there's no way to keep a backwards compatibility with forbidding '_'. If someone has been using "a_b" as a part of specifier "src_%s", this change will break it, so maybe we could extend the number of the specifier on request pad at some point later.
Maybe we could allow multiple conversion specifiers only for %u and %d?
Created attachment 332252 [details] [review] allow multiple conversion specifiers for request pad except '%s' yup, I update the patch not to allow multiple specifiers for '%s'. It's ok to use multiple specifiers and any other characters followed/ahead of the specifier for '%u' and '%d' with this patch
Review of attachment 332252 [details] [review]: Generally looks good to me, might just be a little bit too late for 1.10 now. I'd put it in if others agree though. ::: gst/gstpadtemplate.c @@ +223,3 @@ * element. + * REQUEST padtemplates can have multiple specifiers in case of %d and %u, like + * src_%u_%u, but %s only can be used once in the template. %s should also only be allowed at the very end, while the others can also be in the middle. That's also what the checks for, right?
Created attachment 334573 [details] [review] allow multiple conversion specifiers for request pad except '%s' right, %s should only be allowed at the very end and once in the template name. I added a comment here and test case, also fixed to accept %s following other specifiers.
Comment on attachment 334573 [details] [review] allow multiple conversion specifiers for request pad except '%s' Seems good. Waiting for another opinion if this should go in now or after 1.10.
commit f80dfc9b06d7affe54945cb1486b1c1521f1cb95 Author: Wonchul Lee <wonchul.lee@collabora.com> Date: Fri Apr 29 16:26:49 2016 +0900 element: Allow multiple conversion specifiers for request pads This allows pad template names like "src_%u_%u", but it does not allow multiple specifiers of string type %s as that would lead to ambiguities. https://bugzilla.gnome.org/show_bug.cgi?id=761225