Created attachment 363082 [details] [review]
pad templates: Allow specifying GType

Created attachment 363084 [details] [review]
aggregator: Remove klass->sinkpads_type

This posed problems for the python bindings (and possibly others).

Instead, subclasses now use add_pad_template_with_type.

Review of attachment 363082 [details] [review]:

Makes sense, I would have put the GType into `GstStaticPadTemplate` but it seems not to be possible as the structure doesn't have padding.

`gst_element_class_add_static_pad_template_with_type` is a looong name but well :-)

(In reply to Mathieu Duponchelle from comment #2)
> Created attachment 363084 [details] [review] [review]
> aggregator: Remove klass->sinkpads_type
> 
> This posed problems for the python bindings (and possibly others).
> 
> Instead, subclasses now use add_pad_template_with_type.

Actually this also poses a problem with the python bindings, adding pad templates is already overriden in gst-python so we should just extend it there to be able to also specify a GType.

afaics, the way to do it in python is:

__gsttemplates__ = (Gst.PadTemplate.new("src",
                                           Gst.PadDirection.SRC,
                                           Gst.PadPresence.ALWAYS,
                                           Gst.Caps.new_any())

So there won't be any need for an override if you can do:

__gsttemplates__ = (Gst.PadTemplate.new_with_type("src",
                                           Gst.PadDirection.SRC,
                                           Gst.PadPresence.ALWAYS,
                                           Gst.Caps.new_any(),
                                           your_gtype)

?

That should work indeed :)

I had in my todo something similar, but to be able to print the pad properties. My conclusion was that I needed the GType, for sometimes and request pads, gst-inspect only have access to StaticTemplate, but indeed, there is no padding, that's unfortunate.

This also allows us to introspect pad types, Mathieu said he will add it to gst-inspect :-)

Ok, looking further, we can get the ElementClass from the element, and then iterate over the GstPadTemplates instead of the StaticPadTemplates (which is the only thing available from the registry). That would be great addition indeed.

Good, should we push that then? :)

I guess once the gst-inspect part is done for completeness :)

+1

The only question for me is whether it shouldn't be _with_gtype() and "gtype" property.

What do other libs do? There must be a precedence somewhere.

Comment on attachment 363082 [details] [review]
pad templates: Allow specifying GType

>@@ -127,7 +137,12 @@ struct _GstPadTemplate {
>   GstCaps	  *caps;
> 
>   /*< private >*/
>-  gpointer _gst_reserved[GST_PADDING];
>+  union {
>+    gpointer _gst_reserved[GST_PADDING];
>+    struct {
>+      GType type;
>+    } abi;
>+  } ABI;
> };

For what it's worth, I think we can rely on GType always being pointer-sized, so the ABI union struct compat dance is probably not necessary here, but doesn't hurt either of course.

Created attachment 364206 [details] [review]
pad templates: Allow specifying GType

See https://bugzilla.gnome.org/show_bug.cgi?id=731301

Created attachment 364207 [details] [review]
aggregator: Remove klass->sinkpads_type

This posed problems for the python bindings (and possibly others).

Instead, subclasses now use add_pad_template_with_gtype.

Review of attachment 364206 [details] [review]:

::: docs/gst/gstreamer-sections.txt
@@ +2200,2 @@
 GST_PAD_TEMPLATE_CAPS
 GST_PAD_TEMPLATE_IS_FIXED

Missing the ` GST_PAD_TEMPLATE_TYPE` macro?

::: gst/gstpadtemplate.h
@@ -97,0 +97,9 @@
+/**
+ * GST_PAD_TEMPLATE_TYPE:
+ * @templ: the template to query
... 6 more ...

_GTYPE then? :)

Created attachment 364208 [details] [review]
pad templates: Allow specifying GType

See https://bugzilla.gnome.org/show_bug.cgi?id=731301

(In reply to Thibault Saunier from comment #16)
> Review of attachment 364206 [details] [review] [review]:
> 
> ::: docs/gst/gstreamer-sections.txt
> @@ +2200,2 @@
>  GST_PAD_TEMPLATE_CAPS
>  GST_PAD_TEMPLATE_IS_FIXED
> 
> Missing the ` GST_PAD_TEMPLATE_TYPE` macro?

Yes, good catch

> 
> ::: gst/gstpadtemplate.h
> @@ -97,0 +97,9 @@
> +/**
> + * GST_PAD_TEMPLATE_TYPE:
> + * @templ: the template to query
> ... 6 more ...
> 
> _GTYPE then? :)

Yes, fixed

Review of attachment 364208 [details] [review]:

lgtm

Created attachment 364209 [details] [review]
aggregator: Remove klass->sinkpads_type

This posed problems for the python bindings (and possibly others).

Instead, subclasses now use add_pad_template_with_gtype.

Review of attachment 364209 [details] [review]:

::: gst/mxf/mxfmux.c
@@ -170,3 @@
   gstaggregator_class->stop = GST_DEBUG_FUNCPTR (gst_mxf_mux_stop);
   gstaggregator_class->aggregate = GST_DEBUG_FUNCPTR (gst_mxf_mux_aggregate);
-  gstaggregator_class->sinkpads_type = GST_TYPE_MXF_MUX_PAD;

Isn't it going to use `TYPE_AGGREGATOR_PAD` now?

Review of attachment 364208 [details] [review]:

Commited

Created attachment 364210 [details] [review]
aggregator: Remove klass->sinkpads_type

This posed problems for the python bindings (and possibly others).

Instead, subclasses now use add_pad_template_with_gtype.

Review of attachment 364210 [details] [review]:

lgtm

Attachment 364210 [details] pushed as 56fc5be - aggregator: Remove klass->sinkpads_type