GNOME Bugzilla – Bug 741260
gstpluginfeature: Properly expose the class.
Last modified: 2018-11-03 12:24:38 UTC
To allow external applications libraries to implement their own factory. + Implements getters and setters for priv->plugin and priv->loaded.
Created attachment 292312 [details] [review] gstpluginfeature: Properly expose the class.
Created attachment 292334 [details] [review] gstpluginfeature: Properly expose the class. To allow external applications libraries to implement their own factory. + Implements getters and setters for priv->plugin and priv->loaded.
Review of attachment 292334 [details] [review]: Can you additionally show example code where you use this to implement an external pluginfeature? I'm not convinced this is enough to make external plugin features work properly ::: gst/gstpluginfeature.c @@ +46,3 @@ + guint rank; + const gchar *plugin_name; + GstPlugin *plugin; /* weak ref */ Maybe this should be a GWeakRef nowadays
(In reply to comment #3) > Review of attachment 292334 [details] [review]: > > Can you additionally show example code where you use this to implement an > external pluginfeature? I'm not convinced this is enough to make external > plugin features work properly > > ::: gst/gstpluginfeature.c > @@ +46,3 @@ > + guint rank; > + const gchar *plugin_name; > + GstPlugin *plugin; /* weak ref */ > > Maybe this should be a GWeakRef nowadays Finally found some time to finalize a patch that examplifies inheriting from GstPluginFeature, I added the bug as blocked by this one ( https://bugzilla.gnome.org/show_bug.cgi?id=742610 ) A test is added in the validate's test suite if you want to give the thing a try :)
The relevant part is in gst-validate-override-factory.c by the way.
Review of attachment 292334 [details] [review]: +1 for the idea. I think having api for this also simplifies the internal code (e.g. the weak pointer). ::: gst/gstpluginfeature.c @@ +197,3 @@ + * @feature: feature to set plugin on + * @plugin: Set the plugin that provides this feature + * missing doc line. @@ +204,3 @@ + g_return_if_fail (feature != NULL); + g_return_if_fail (GST_IS_PLUGIN_FEATURE (feature)); + * @plugin: Set the plugin that provides this feature wouldn't you need to g_object_remove_weak_pointer () if feature->priv->plugin != NULL @@ +210,3 @@ + feature->priv->plugin_name = plugin->desc.name; + } else { +void do we need this? ::: gst/gstregistry.c @@ +752,3 @@ + + rank1 = gst_plugin_feature_get_rank (GST_PLUGIN_FEATURE (fac1)); + rank2 = gst_plugin_feature_get_rank (GST_PLUGIN_FEATURE (fac2)); nit: guint rank1 = gst_plugin_feature_get_rank (GST_PLUGIN_FEATURE (fac1)); guint rank2 = gst_plugin_feature_get_rank (GST_PLUGIN_FEATURE (fac2));
As mentioned on IRC, I'm not keen on exposing this at all, but I haven't looked at your use case yet, so I'll reserve judgement until I have :)
(In reply to comment #6) > Review of attachment 292334 [details] [review]: > > +1 for the idea. I think having api for this also simplifies the internal code > (e.g. the weak pointer). > The code was indeed not very well encapsulated in that regard, admittedly we could have this API and still hide PluginFeature but :) > ::: gst/gstpluginfeature.c > @@ +197,3 @@ > + * @feature: feature to set plugin on > + * @plugin: Set the plugin that provides this feature > + * > > missing doc line. Fixed. > @@ +204,3 @@ > + g_return_if_fail (feature != NULL); > + g_return_if_fail (GST_IS_PLUGIN_FEATURE (feature)); > + * @plugin: Set the plugin that provides this feature > > wouldn't you need to g_object_remove_weak_pointer () if feature->priv->plugin > != NULL > Indeed, done, I'm not sure of whether there was a case where we would reset a different plugin but can't hurt. > @@ +210,3 @@ > + feature->priv->plugin_name = plugin->desc.name; > + } else { > +void > > do we need this? > Not really, don't want to be too intrusive though. > ::: gst/gstregistry.c > @@ +752,3 @@ > + > + rank1 = gst_plugin_feature_get_rank (GST_PLUGIN_FEATURE (fac1)); > + rank2 = gst_plugin_feature_get_rank (GST_PLUGIN_FEATURE (fac2)); > > nit: > guint rank1 = gst_plugin_feature_get_rank (GST_PLUGIN_FEATURE (fac1)); > guint rank2 = gst_plugin_feature_get_rank (GST_PLUGIN_FEATURE (fac2)); Style consideration, ain't got nothing against that, done ;) Fixed patch attached next, waiting for tim's eyeballs on our use case :)
Created attachment 294318 [details] [review] gstpluginfeature: Properly expose the class. To allow external applications libraries to implement their own factory. + Implements getters and setters for priv->plugin and priv->loaded.
What should we do about that bug? Any objection to get that in? We do need it for -validate :) (I would be annoyed to have to copy that code inside -validate...)
This should not get in until properly reviewed by more people, and we should also do it after 1.6 as we don't have much time to fix anything that appears otherwise... and would have to live with the consequences for a very long time. Just exposing this internals is not that trivial, and might make it more difficult later to implement things better and cleaner.
Well there sort of had been three reviewers already (counting thib because we discussed implementation on our side). Not merging it will make it more difficult for us to implement things better and cleaner in validate :/
Yes, I object to getting this in, at least for 1.6 :) Reasons: - I think there's not enough time now to stabilise and test (even if you've been testing it) - I think there might be some issues with the way the registry works, in that the GType for all plugin feature subclasses supported needs to be registered at the time the registry is loaded (are you using a different GstRegistry that looks in a different path in your case? How does deserialisation work for your subclass?) - I don't really want to make GstRegistry something other projects should use. I'd like to hide it even more in fact :) Would prefer other projects used libpeas or a registry copy. - I have some quite invasive refactorings of the GstRegistry stuff planned, and making this public now would probably interfere with that.
So be it, we'll have a look at libpeas then. Encapsulating is still nice though, might be worth a patch, even if the functions are only exposed privately, tell me if I should take 5 minutes to do that :)
I'll answer these btw (In reply to comment #13) > - I think there's not enough time now to stabilise and test (even if you've > been testing it) > "Stabilizing" this small changeset doesn't seem out of reach for me :P > - I think there might be some issues with the way the registry works, in that > the GType for all plugin feature subclasses supported needs to be registered at > the time the registry is loaded (are you using a different GstRegistry that > looks in a different path in your case? How does deserialisation work for your > subclass?) The registry is a different one, no serialization / deserialization > - I don't really want to make GstRegistry something other projects should use. > I'd like to hide it even more in fact :) Would prefer other projects used > libpeas or a registry copy. > Indeed, we'll have a look at libpeas > - I have some quite invasive refactorings of the GstRegistry stuff planned, > and making this public now would probably interfere with that. What do you intend to do?
> The registry is a different one, no serialization / deserialization What do you mean by this? Could you elaborate?
As explained on IRC, we don't need to serialize / deserialize because we reload all plugins and disable the registry fork. We also won't use libpeas, it's not adapted to our use case.
-- 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/85.