GNOME Bugzilla – Bug 779344
plugin: Unify static and dynamic plugin interface
Last modified: 2017-05-09 12:51:37 UTC
The following patch is a proposal to unify the build of static and dynamic plugins. This would allow to remove the duplication in cerbero, and symplify the build for static and dynamic plugin support.
Created attachment 346879 [details] [review] plugin: Unify static and dynamic plugin interface This patch changes the entry point of each plugin in order to unify the interface for static and dynamic plugin. What we do is replace the current static plugin interface and extend the dymamic one. The plugin entry was a C structure, name "gst_plugin_desc". With this patch, the interface is now: GstPpluginDesc *gst_plugin_<name>_get_desc(void); The reason we change the C structure into function, is that it is potentially more common to have function pointers, avoiding possible binding language limitation. Additionally to that. This change prevents the symbols from clashing between plugins, allowing to build once the plugin (assuming you have -fPIC). On the plugin loader side, we symply derive the shared object basename to extract the plugin name. If this symbol is not found, we fallback to gst_plugin_desc for backward compatibility. This has one side effect, which is that the shared objects now need to be named after their plugin name. This is generally the case with few exceptions. The benifit of this limitation is that you can control the gst_plugin_<name>_desc clash at file level.
Here's an initial list of plugin shared object that need to be renamed. We can also remove related hack in the Android build scripts (there might be more, I am using meson right now). camerabin2 encodebin fluidsynth gtk kmssink onvif pulse rmdemux sdpelem shout2 siren souphttpsrc validateplugin xvimage On meson side at least, this is trivial, few more lines on the Makefile.am, but nothing complicated. Plugin names isn't part of the ABI, so on this side this should also not be a problem.
Review of attachment 346879 [details] [review]: Makes sense to me, we just need to update the Android cerbero build at the same time. ::: gst/gstplugin.h @@ +252,3 @@ +GST_PLUGIN_EXPORT void G_PASTE(gst_plugin_, G_PASTE(name, _register)) (void); \ +\ +static GstPluginDesc gst_plugin_desc = { \ const? @@ +268,3 @@ +\ +GstPluginDesc * \ +G_PASTE(gst_plugin_, G_PASTE(name, _get_desc)) (void) \ And const here? :)
(In reply to Sebastian Dröge (slomo) from comment #3) > Review of attachment 346879 [details] [review] [review]: > > Makes sense to me, we just need to update the Android cerbero build at the > same time. > > ::: gst/gstplugin.h > @@ +252,3 @@ > +GST_PLUGIN_EXPORT void G_PASTE(gst_plugin_, G_PASTE(name, _register)) > (void); \ > +\ > +static GstPluginDesc gst_plugin_desc = { \ > > const? Right, I started with that, but removed it, as I wanted to know first it that was doable. I'll fix that in next version, it will require adding const all over the place inside gstplugin.c. > > @@ +268,3 @@ > +\ > +GstPluginDesc * \ > +G_PASTE(gst_plugin_, G_PASTE(name, _get_desc)) (void) \ > > And const here? :) Same.
Created attachment 347015 [details] [review] [v2] plugin: Unify static and dynamic plugin interface v2 note: GstPluginDesc are now const. This patch changes the entry point of each plugin in order to unify the interface for static and dynamic plugin. What we do is replace the current static plugin interface and extend the dymamic one. The plugin entry was a C structure, name "gst_plugin_desc". With this patch, the interface is now: GstPpluginDesc *gst_plugin_<name>_get_desc(void); The reason we change the C structure into function, is that it is potentially more common to have function pointers, avoiding possible binding language limitation. Additionally to that. This change prevents the symbols from clashing between plugins, allowing to build once the plugin (assuming you have -fPIC). On the plugin loader side, we symply derive the shared object basename to extract the plugin name. If this symbol is not found, we fallback to gst_plugin_desc for backward compatibility. This has one side effect, which is that the shared objects now need to be named after their plugin name. This is generally the case with few exceptions. The benifit of this limitation is that you can control the gst_plugin_<name>_desc clash at file level.
Created attachment 347205 [details] [review] Fix plugin filename to match pugin name - libgstencodebin.so is now libgstencoding.so - libgstximage.so is now libgstximagesink.so (meson only)
Created attachment 347207 [details] [review] Fix plugin filename to match pugin name - libgstencodebin.so is now libgstencoding.so - libgstximage.so is now libgstximagesink.so (meson only)
Created attachment 347212 [details] [review] Fix plugin filename to match pugin name - libgstencodebin.so is now libgstencoding.so - libgstximage.so is now libgstximagesink.so (meson only)
Created attachment 347213 [details] [review] Fix plugin filenames to match plugin names - libgstpulse.so becomes libgstpulseaudio.so - libgstsouphttpsrc.so becomes libgstsoup.so - libgstoss4audio.so becomes libgstoss4.so
Created attachment 347214 [details] [review] Fix plugin filenames to match pugin names - libgstencodebin.so is now libgstencoding.so - libgstximage.so is now libgstximagesink.so (meson only)
Created attachment 347217 [details] [review] Rename plugin filesnames to match plugin names - libgstgtksink.so -> libgstgtk.so - libgstteletextdec.so -> libgstteletex.so - libgstcamerabin2.so -> libgstcamerabin.so - libgstonvif.so -> libgstrtponvif.so (meson only) - sdp -> sdpelem (avoid clash with libgstsdp) - gstsiren -> siren - libgstkmssink.so -> libgstkms.so
Created attachment 347218 [details] [review] Rename plugin filenames to match plugin names libgstrmdemux.so becomes libgstrealmedia.so
Created attachment 347219 [details] [review] Rename plugin filenames to match plugin names - libgstvalidateplugin.so -> libgstvalidatetracer.so - faultinjection -> validatefaultinjection - gstvalidategtk -> validategtk - ssim -> validatessim
I'd suggest that we get all the renames in for 1.12 already as they make things more consistent in general, and consider the other part after 1.12. This would then also allow people to relatively easily backport the other part to 1.12 if they want to. Thoughts?
I'm currently testing patches on cerbero that removes the -static, I can probably do the in two steps indeed. One question, on Windows, static build support adds funky ifdec and cdecl dance. This is badly documented, and I think there is some deadcode in it to add up. If someone can explain, might be problematic for my main goal. Though, if it is, then it means we also need to build the rest twice for static on Windows, which would indicate that it does not fully work anyway.
Review of attachment 347212 [details] [review]: LGTM besides that ::: docs/plugins/inspect/plugin-libvisual.xml @@ +179,3 @@ </element> </elements> +</plugin> Could you remove this change? Seems to be just noise. :)
Review of attachment 347213 [details] [review]: ::: docs/plugins/inspect/plugin-oss4.xml @@ +7,3 @@ <license>LGPL</license> <source>gst-plugins-good</source> + <package>GStreamer Good Plug-ins git</package> Looks like this is "source release" everywhere else? ::: docs/plugins/inspect/plugin-pulseaudio.xml @@ +7,3 @@ <license>LGPL</license> <source>gst-plugins-good</source> + <package>GStreamer Good Plug-ins git</package> Ditto ::: docs/plugins/inspect/plugin-shout2send.xml @@ +7,3 @@ <license>LGPL</license> <source>gst-plugins-good</source> <package>libshout2</package> <package> here seems to be wrong; please change it to "GStreamer Good Plug-ins source release" since you're touching this file anyway ;) @@ +21,3 @@ <direction>sink</direction> <presence>always</presence> + <details>application/ogg; audio/ogg; video/ogg; audio/mpeg, mpegversion=(int)1, layer=(int)[ 1, 3 ]</details> This change is incorrect; the plugin does support webm when libshout >= 2.3.0; grep for "SHOUT_FORMAT_WEBM". Ideally this should be changed at build-time, but I don't see how...
Review of attachment 347214 [details] [review]: Duplicate patch? :)
Review of attachment 347217 [details] [review]: ::: docs/plugins/inspect/plugin-gtk.xml @@ +7,3 @@ + <license>LGPL</license> + <source>gst-plugins-bad</source> + <package>GStreamer Bad Plug-ins git</package> Should be "Plug-ins source release", according to other files. ::: docs/plugins/inspect/plugin-sdp.xml @@ +7,3 @@ <license>LGPL</license> <source>gst-plugins-bad</source> + <package>GStreamer Bad Plug-ins git</package> Ditto. ::: gst/sdp/gstsdpelem.c @@ +39,3 @@ GST_PLUGIN_DEFINE (GST_VERSION_MAJOR, GST_VERSION_MINOR, + sdpelem, Maybe this should be the other way around? Change the plugin name to be `libgstsdp.so`? For consistency with how other plugins are named.
(In reply to Nicolas Dufresne (stormer) from comment #12) > Created attachment 347218 [details] [review] [review] > Rename plugin filenames to match plugin names > > libgstrmdemux.so becomes libgstrealmedia.so Is this change not needed or did you obsolete this by mistake?
(In reply to Nicolas Dufresne (stormer) from comment #15) > One question, on Windows, static build support adds funky ifdec and cdecl > dance. This is badly documented, and I think there is some deadcode in it to > add up. If someone can explain, might be problematic for my main goal. > Though, if it is, then it means we also need to build the rest twice for > static on Windows, which would indicate that it does not fully work anyway. What are you referring to here? Do you mean __declspec in gstconfig.h.in or something else?
The docs/inspect/*xml files are autogenerated, they should not be edited manually, and we shouldn't worry too much about noise/changes there. If there are things that are not right, they will usually have to be fixed in the plugin metadata. The package string changes between git and proper releases, it will update automatically as well, not important if it matches, it will be updated before a release.
Exactly, though we should file a bug for the shout package, seems odd. About Windows, their is an export macro being tweaked for static vs dynamic plugins, if you really need that for static plugin, you need that for libraries too, which I doubt. Anyway, when merged, you'll be able to figure-out.
So I just removed the need-work as all the review comments are about the generated xml, the "from git" will be back to release upon next release, as I expect "make update" to be run for each major version. About the snippet that I'm worried about: >/* Only use __declspec(dllexport/import) when we have been built with MSVC or > * the user is linking to us with MSVC. The only remaining case is when we were > * built with MinGW and are linking with MinGW in which case we rely on the > * linker to auto-export/import symbols. Of course all this is only used when > * not linking statically. > * > * NOTE: To link to GStreamer statically on Windows, you must define > * GST_STATIC_COMPILATION or the prototypes will cause the compiler to search > * for the symbol inside a DLL. > */ >#if (@GSTCONFIG_BUILT_WITH_MSVC@ || defined(_MSC_VER)) && >!defined(GST_STATIC_COMPILATION) ># define GST_PLUGIN_EXPORT __declspec(dllexport) ># ifdef GST_EXPORTS ># define GST_EXPORT __declspec(dllexport) ># else ># define GST_EXPORT __declspec(dllimport) extern ># endif >#else ># define GST_PLUGIN_EXPORT ># if (defined(__SUNPRO_C) && (__SUNPRO_C >= 0x590)) ># define GST_EXPORT extern __attribute__ ((visibility ("default"))) ># else ># define GST_EXPORT extern ># endif >#endif Re-reading this, I can only conclude that with MSVC, you can build static or dynamic, but not both at the same time, while with MingW it's the same as with GCC / CLANG. So basically, with MSVC, you need to build GStreamer twice if you want both static and dynamic, the -static recipes were not helping much.
Created attachment 347357 [details] [review] Fix the build according to the plugin rename Plugins have been rename so their library name now matches their plugin name. This is the final missing peace for merging the rename. I'm doing more local test with this patch, if it works for me, I'll push and let the build both test the rest.
Comment on attachment 347214 [details] [review] Fix plugin filenames to match pugin names Attachment 347214 [details] pushed as fb7d9e2 - Fix plugin filenames to match pugin names
Comment on attachment 347213 [details] [review] Fix plugin filenames to match plugin names Attachment 347213 [details] pushed as ca0ed8a - Fix plugin filenames to match plugin names
Comment on attachment 347217 [details] [review] Rename plugin filesnames to match plugin names Attachment 347217 [details] pushed as eb2dae8 - Rename plugin filesnames to match plugin names
Comment on attachment 347219 [details] [review] Rename plugin filenames to match plugin names Attachment 347219 [details] pushed as c1e526a - Rename plugin filenames to match plugin names
There is one on gst-devtools: commit bdd152484c9900fb803fc4cf8038324f116e9db6 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Sat Mar 4 11:13:33 2017 -0500 Rename plugin filenames to match plugin names - libgstvalidateplugin.so -> libgstvalidatetracer.so - faultinjection -> validatefaultinjection - gstvalidategtk -> validategtk - ssim -> validatessim https://bugzilla.gnome.org/show_bug.cgi?id=779344
Comment on attachment 347357 [details] [review] Fix the build according to the plugin rename Oops, this one lost it's bug reference, sorry: commit c646b009429f98c81e1370b44a69131cb93cfe81 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon Mar 6 21:22:15 2017 -0500 Fix the build according to the plugin rename Plugins have been rename so their library name now matches their plugin name.
Created attachment 347518 [details] [review] Remove duplicated static gstreamer build This patch goes with attachment 347015 [details] [review], it's the next step, which consist of actually removing the -static plugin.
OK, so let's the CI do the work now, I'll monitor and fix any issues that appear.
So, apparently I broke doc make distcheck, it fails in when running xsltproc. We have a bug in our build system, warning and errors are hidden. Running manually showed: /usr/bin/xsltproc --nonet --xinclude --stringparam gtkdoc.bookname gst-plugins-good-plugins-1.0 --stringparam gtkdoc.version 1.25 --stringparam chunk.quietly 1 --stringparam chunker.output.quiet 1 /usr/share/gtk-doc/data/gtk-doc.xsl gst-plugins-good-plugins-docs.sgml warning: failed to load external entity "../../../tests/examples/audiofx/firfilter-example.c" xml/element-audiofirfilter.xml:89: element include: XInclude error : could not load ../../../tests/examples/audiofx/firfilter-example.c, and no fallback was found warning: failed to load external entity "../../../tests/examples/audiofx/iirfilter-example.c" xml/element-audioiirfilter.xml:85: element include: XInclude error : could not load ../../../tests/examples/audiofx/iirfilter-example.c, and no fallback was found warning: failed to load external entity "../../../tests/examples/level/level-example.c" xml/element-level.xml:132: element include: XInclude error : could not load ../../../tests/examples/level/level-example.c, and no fallback was found warning: failed to load external entity "../../../tests/examples/spectrum/spectrum-example.c" xml/element-spectrum.xml:131: element include: XInclude error : could not load ../../../tests/examples/spectrum/spectrum-example.c, and no fallback was found warning: failed to load external entity "xml/plugin-shout2send.xml" gst-plugins-good-plugins-docs.sgml:346: element include: XInclude error : could not load xml/plugin-shout2send.xml, and no fallback was found
The reminds me, I need to add some cruft to the list, I think the xml/plugin-shout2send.xml is good candidate. For the libgst... stuff, it's much harder, since the list is huge.
-good: commit 7d2cf928abd5784ba8dc3a73f0eb0fc9e61ba601 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Mar 8 22:15:46 2017 -0500 Fix shout2 plugin doc generation In the previous patch, we also renamed shout2send to shout2, so it does not clash with it's feature. Though we forgot to rename it in the doc reference. This patch also add a cruft detection on the xml that made me miss this error. https://bugzilla.gnome.org/show_bug.cgi?id=779344 -bad commit 8ae5eaec055820e31c1f0b1670b18210ab208fda Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Mar 8 22:28:15 2017 -0500 Fix doc generation This regression was caused by the renaming of plugin-sdp into plugin-sdpelem. The doc reference needed an update. Also, add the old xml to the cruft file list. https://bugzilla.gnome.org/show_bug.cgi?id=779344
All the old plugin filenames must be added to the CLEAN_CRUFT stuff
Yes, next step, only .so files are needed (for uninstalled setup). I doubt adding cruft path to the selected prefix (for installed setup) is realistic, is it? Another question, while at it, shell I simply remove all the clash between plugin name and their feature? (E.g ximagesink).
No, just like the ones that are already there. The cruft stuff is only for uninstalled. See e.g. rawparse in -bad (which become legacyrawparse)
-base commit e4b7b10eed82010a689cce451e0dd18fe348eca2 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Sun Mar 12 11:35:25 2017 -0400 Add old libgstencodebin.so to cruft list This will help fixing clash in gst-uninstalled setup. https://bugzilla.gnome.org/show_bug.cgi?id=779344 -good commit 91080c4804269abd6130585e78d7dacd8633d469 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Sun Mar 12 11:42:25 2017 -0400 Add old plugin names to cruft list This will help fixing uninstalled setup. Also fix missing path correction in one of the plugin xml. https://bugzilla.gnome.org/show_bug.cgi?id=779344 -bad commit 424dd98bdae61a3e162060d05281859455bfce90 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Sun Mar 12 11:50:50 2017 -0400 Add old plugins names to cruft list This helps fixing uninstalled setup. Also fixes some path in plugin xml files. https://bugzilla.gnome.org/show_bug.cgi?id=779344 -ugly commit fff38f7f97b74f2c1689728dd570c058b9bb39ec Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Sun Mar 12 11:57:23 2017 -0400 Add libgstrmdemux.so to cruft list This was renamed to libgstrealmedia.so, this fixes helps fixing uninstalled setup. https://bugzilla.gnome.org/show_bug.cgi?id=779344
*** Bug 747917 has been marked as a duplicate of this bug. ***
Created attachment 351430 [details] [review] libav: Allow build both static dynamic plugins When building plugins with internal FFMPEG, we use different link flags depending if it is static or shared. As we want to build both static and dynamic plugins at once, rewrite the rules so we can pass the right flags.
Comment on attachment 351430 [details] [review] libav: Allow build both static dynamic plugins Attachment 351430 [details] pushed as dcc36f6 - libav: Allow build both static dynamic plugins
Attachment 347518 [details] pushed as daa9187 - Remove duplicated static gstreamer build
And this one too: commit e7ede5a487d8c1571a4387a5f8eeb4d5e73a9553 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon Feb 27 21:38:11 2017 -0500 plugin: Unify static and dynamic plugin interface