After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 761225 - pad: Allow multiple conversion specifiers for request pads and don't require it at the very end
pad: Allow multiple conversion specifiers for request pads and don't require ...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other Linux
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-01-28 08:13 UTC by Wonchul Lee
Modified: 2016-11-01 18:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[patch] Allow multiple conversion specifiers for request pads (13.78 KB, patch)
2016-02-04 07:54 UTC, Wonchul Lee
none Details | Review
[patch] Allow multiple conversion specifiers for request pads 2 (16.18 KB, patch)
2016-02-09 08:32 UTC, Wonchul Lee
none Details | Review
[patch] Allow multiple conversion specifiers for request pads 3 (16.18 KB, patch)
2016-03-29 13:54 UTC, Wonchul Lee
none Details | Review
allow multiple conversion specifiers for request pad 4 (24.25 KB, patch)
2016-05-02 06:41 UTC, Wonchul Lee
none Details | Review
allow multiple conversion specifiers for request pad 5 (20.05 KB, patch)
2016-06-22 03:07 UTC, Wonchul Lee
none Details | Review
allow multiple conversion specifiers for request pad (20.48 KB, patch)
2016-07-07 01:53 UTC, Wonchul Lee
none Details | Review
allow multiple conversion specifiers for request pad except '%s' (22.88 KB, patch)
2016-07-28 05:45 UTC, Wonchul Lee
none Details | Review
allow multiple conversion specifiers for request pad except '%s' (23.41 KB, patch)
2016-09-01 01:09 UTC, Wonchul Lee
committed Details | Review

Description Wonchul Lee 2016-01-28 08:13:22 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.
Comment 1 Sebastian Dröge (slomo) 2016-01-28 08:34:22 UTC
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.
Comment 2 Wonchul Lee 2016-01-28 08:42:37 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2016-01-28 09:02:15 UTC
No, but you should be able to create a request pad template with that name. Is it not possible?
Comment 4 Sebastian Dröge (slomo) 2016-01-28 09:03:34 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2016-01-28 09:06:05 UTC
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.
Comment 6 Wonchul Lee 2016-01-28 12:00:54 UTC
yeah, If it's acceptable, I'm willing to write a patch to make the restriction less strong :)
Comment 7 Sebastian Dröge (slomo) 2016-01-28 12:24:09 UTC
Well, you'll have to check all the code that currently does assumptions about this and check if it can be changed.
Comment 8 Nicolas Dufresne (ndufresne) 2016-01-28 13:11:13 UTC
Maybe the checks are correct, but need to be part of overridable code ?
Comment 9 Sebastian Dröge (slomo) 2016-01-29 09:38:14 UTC
There's nothing really overridable here. It's code for automatically selecting the appropriate request pad template when linking elements together.
Comment 10 Nicolas Dufresne (ndufresne) 2016-01-29 13:57:32 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2016-01-29 14:20:31 UTC
Ideally it would be changed to work with both variants, but yes that would also be an option.
Comment 12 Wonchul Lee 2016-02-04 07:54:51 UTC
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.
Comment 13 Sebastian Dröge (slomo) 2016-02-05 00:00:27 UTC
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).
Comment 14 Sebastian Dröge (slomo) 2016-02-05 00:18:10 UTC
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.
Comment 15 Wonchul Lee 2016-02-09 08:32:04 UTC
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.
Comment 16 Sebastian Dröge (slomo) 2016-03-29 07:39:17 UTC
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
Comment 17 Wonchul Lee 2016-03-29 13:54:43 UTC
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)
Comment 18 Wonchul Lee 2016-03-29 13:55:27 UTC
and I fixed convention -> conversion :)
Comment 19 Sebastian Dröge (slomo) 2016-04-03 09:02:17 UTC
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
Comment 20 Wonchul Lee 2016-05-02 06:41:13 UTC
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.
Comment 21 Wonchul Lee 2016-06-22 03:07:34 UTC
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.
Comment 22 Wonchul Lee 2016-07-07 01:53:52 UTC
Created attachment 330977 [details] [review]
allow multiple conversion specifiers for request pad

ping
Comment 23 Sebastian Dröge (slomo) 2016-07-07 07:49:14 UTC
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.
Comment 24 Wonchul Lee 2016-07-08 02:23:57 UTC
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.
Comment 25 Sebastian Dröge (slomo) 2016-07-08 08:20:54 UTC
How should forbidding '_' as part of %s be done in a backwards compatible way?
Comment 26 Wonchul Lee 2016-07-27 00:54:05 UTC
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.
Comment 27 Sebastian Dröge (slomo) 2016-07-27 06:30:19 UTC
Maybe we could allow multiple conversion specifiers only for %u and %d?
Comment 28 Wonchul Lee 2016-07-28 05:45:36 UTC
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
Comment 29 Sebastian Dröge (slomo) 2016-08-31 21:12:26 UTC
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?
Comment 30 Wonchul Lee 2016-09-01 01:09:02 UTC
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 31 Sebastian Dröge (slomo) 2016-09-01 06:21:44 UTC
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.
Comment 32 Sebastian Dröge (slomo) 2016-11-01 18:30:17 UTC
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