GNOME Bugzilla – Bug 510187
gst_plugin_register_static() API review and GST_PLUGIN_DEFINE_STATIC deprecation
Last modified: 2008-01-20 15:05:59 UTC
Recent postings to gstreamer-devel suggest that the newly-introduced gst_plugin_register_static() API and the uncompromising deprecation of the GST_PLUGIN_DEFINE_STATIC macro may not be ideal in their current state and should at least be reviewed before release. Two issues: (a) when -DGST_DISABLE_DEPRECATED is used, the compiler will throw a bunch of somewhat cryptic errors that aren't particularly helpful and don't indicate that the macro is not defined any longer because it has been deprecated. I can think of three possible resolutions: (a1) tough luck, that's what you get for compiling with that flag (ie. leave it as it is now) (a2) do something along the lines of: ... #else /* GST_DISABLE_DEPRECATED */ #define FOO_STATIC(...) FOO_STATIC_has_been_deprecated #endif /* GST_DISABLE_DEPRECATED */ (a3) don't wrap GST_PLUGIN_DEFINE_STATIC with #ifndef GST_DISABLE_DEPRECATED at all, in which case the compiler will just warn about the old _gst_plugin_register_static() function not being declared but probably continue. (b) is the current gst_plugin_register_static() API ok or is it confusing? GST_PLUGIN_DEFINE_STATIC automatically inserts a PACKAGE argument into the GstPluginDesc struct, which most people will probably forget when converting the arguments to GST_PLUGIN_DEFINE_STATIC to a GstPluginDesc, which will result in the plugin not getting registered. Three solutions: (b1) leave it as it is (b2) add a GST_PLUGIN_DESC macro similar to GST_ELEMENT_DETAILS which either adds PACKAGE automatically or adds arguments for the padding so that the compiler will complain. [yuk] (b3) make gst_plugin_register_static() take the stuff in the GstPluginDesc struct as arguments rather than a GstPluginDesc struct. This is more intuitive, easier to document and the compiler will complain about missing arguments. I'm leaning towards (a3) and (b3). I'll make a patch as soon as there's a consensus what to do (if anything needs changing at all). Opinions?
bit undecided about (a2)/(a3), but definitely infavour of (b3). regarding (a) I am afraid that gtk-doc might complain about missing deprecations guards in the case of (a3).
> I am afraid that gtk-doc might complain about missing > deprecations guards in the case of (a3). Will that break our build or will it just complain? If the former, we could probably fool it by using #ifndef .. #else ... #endif and using the same define in both branches, no?
It won't fail. And if we're using (a2) we won't get a warning, as we have deprecation guards when we deprecate it and the legacy code is in the #else branch.
give me a patch that works. 2nd pre-release tarballs are tonight. If there's still questions about whether to deprecate the macro or not at this stage, I say: (a4) Don't deprecate anything in this release, punt it for the next. as for the new gst_plugin_register_static() API, I like b3 too, even though it means changing the places where the new API is already being used.
Created attachment 103072 [details] [review] Change gst_plugin_register_static() to take everything as arguments directly instead of a GstPluginDesc We only need to change core for this, I don't think it's used anywhere else (the other unit tests just use gst_element_register etc. with a NULL plugin).
It will become apparent pretty quickly if anyone has used it already, and is easy to fix. Those changes look good to me, but what about the DEPRECATED guard thing?
Created attachment 103076 [details] [review] remove deprecation guards around GST_PLUGIN_DEFINE_STATIC I think this is the best solution, since it will only break the build if people compile with -DGST_DISABLE_DEPRECATED and -Werror (in which case it's ok to break IMO). It's the best we can do while still deprecating the macro. Don't really see the point of putting it off, the problem will be the same for the next release.
boffo
Committed, thanks (and sorry for the mess): 2008-01-17 Tim-Philipp Müller <tim at centricular dot net> * gst/gstplugin.h: (GST_PLUGIN_DEFINE_STATIC): Remove deprecation guards around GST_PLUGIN_DEFINE_STATIC. This makes gtk-doc complain, but results in slightly better compiler errors. The old _gst_plugin_register_static() is still guarded, so there'll be a compiler warning about that instead. Fixes #510187 too.
This is not over yet. I screwed up: - _gst_plugin_initialize() still does: g_list_foreach (static_plugins, (GFunc) gst_plugin_register_static, NULL); which assumes the old function signature (oops). So programs using the old GST_PLUGIN_DEFINE_STATIC won't get their static plugins registered any longer (and that's the best case scenario). - _gst_plugin_register_static() was changed to do the technically Right Thing, namely call g_thread_init() before calling GLib functions such as g_list_prepend(). We can't really do this, however, since there are a lot of apps out there who just call g_thread_init(NULL) first thing in main() without guarding it with an if (!g_thread_supported()). GLib will abort if g_thread_init() is called twice. Even if the fault is really with the application, we can't really do this. Solution: don't use GLib functions to store the list of plugins to register later when gst_init() is called. Better use plain libc malloc and not use g_thread_init() here at all. Patches coming up.
Created attachment 103223 [details] [review] don't call g_thread_init() in _gst_plugin_register_static/pre-main() constructor; fix _gst_plugin_initialize() for new gst_plugin_register_static() signature; small unit test
Thinking long-term, we should add a new, sane replacement for g_thread_init() to glib.
Well, that's what pre-releases are for. I don't understand this line in the test-case: fail_unless (plugin_init_counter == 0 || plugin_init_counter == 3); Why allow it to succeed if the plugins didn't get registered? Also, the other stuff about testing g_thread_supported() in the plugin_inits that I asked on IRC. The gstplugin bit looks fine, but not the testcase.
> I don't understand this line in the test-case: > > fail_unless (plugin_init_counter == 0 || plugin_init_counter == 3); > > Why allow it to succeed if the plugins didn't get registered? In case GST_PLUGIN_DEFINE_STATIC doesn't work because the compiler doesn't support constructors. > Also, the other stuff about testing g_thread_supported() in the plugin_inits > that I asked on IRC. Yeah, those don't make sense.
(In reply to comment #14) > > I don't understand this line in the test-case: > > > > fail_unless (plugin_init_counter == 0 || plugin_init_counter == 3); > > > > Why allow it to succeed if the plugins didn't get registered? > > In case GST_PLUGIN_DEFINE_STATIC doesn't work because the compiler doesn't > support constructors. But but but but then what exactly is the test testing? You'd know if the compiler didn't support constructors, because it wouldn't compile, right? > > Also, the other stuff about testing g_thread_supported() in the plugin_inits > > that I asked on IRC. > > Yeah, those don't make sense. >
Created attachment 103251 [details] [review] updated patch The unit test just makes sure that the old code path via _gst_element_register_static() still works and doesn't do anything dumb (like assuming a wrong function signature when using g_list_foreach). e.g if you change _static_plugins[_num_static_plugins - 1] = *desc; to _static_plugins[_num_static_plugins] = *desc; it will fail at least under valgrind. If the compiler doesn't support constructors, things should still compile, although I'm not sure if that's true for -Wall -Werror. I've changed the test to only test this stuff with gcc for now, since that's the only compiler for which we define GST_GNUC_CONSTRUCTOR.
Thanks, committed: 2008-01-20 Tim-Philipp Müller <tim at centricular dot net> * gst/gstplugin.c: (_gst_plugin_initialize): Fix old-style static plugins via GST_PLUGIN_DEFINE_STATIC again, which I broke two commits ago when changing the API of gst_plugin_register_static(): the g_list_foreach() in _gst_plugin_register_static still assumed the old function signature and would therefore fail (re-fixes #510187). * gst/gstplugin.c: (_num_static_plugins), (_static_plugins), (_gst_plugin_register_static), (gst_plugin_register_static): Revert the (technically correct) change to call g_thread_init() from the pre-main() constructor. This will break programs which call g_thread_init() without an if (!g_thread_supported()) guard in their main function. We could just blame it on GLib or the application, but it's probably best to just avoid this altogether and simply not use any GLib functions here and use plain old malloc() with a simple array to store the plugins to register later when gst_init() is finally called (re-fixes #510187). * tests/check/gst/gstplugin.c: (GST_GNUC_CONSTRUCTOR_DEFINED), (GST_GNUC_CONSTRUCTOR_DEFINED), (plugin_init_counter), (plugin1_init), (plugin2_init), (plugin3_init), (GST_START_TEST), (GST_START_TEST), (gst_plugin_suite): Dumb unit test to make sure the old GST_PLUGIN_DEFINE_STATIC still works. Let's hope it's all good now.