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 510187 - gst_plugin_register_static() API review and GST_PLUGIN_DEFINE_STATIC deprecation
gst_plugin_register_static() API review and GST_PLUGIN_DEFINE_STATIC deprecation
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal blocker
: 0.10.16
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-01-17 14:21 UTC by Tim-Philipp Müller
Modified: 2008-01-20 15:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Change gst_plugin_register_static() to take everything as arguments directly instead of a GstPluginDesc (7.66 KB, patch)
2008-01-17 16:50 UTC, Tim-Philipp Müller
committed Details | Review
remove deprecation guards around GST_PLUGIN_DEFINE_STATIC (1.28 KB, patch)
2008-01-17 17:08 UTC, Tim-Philipp Müller
committed 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 (5.51 KB, patch)
2008-01-19 21:38 UTC, Tim-Philipp Müller
needs-work Details | Review
updated patch (5.11 KB, patch)
2008-01-20 12:18 UTC, Tim-Philipp Müller
committed Details | Review

Description Tim-Philipp Müller 2008-01-17 14:21:03 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?
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2008-01-17 14:32:48 UTC
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).
Comment 2 Tim-Philipp Müller 2008-01-17 14:52:13 UTC
> 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?
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2008-01-17 15:11:30 UTC
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.
Comment 4 Jan Schmidt 2008-01-17 15:20:48 UTC
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.
Comment 5 Tim-Philipp Müller 2008-01-17 16:50:44 UTC
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).
Comment 6 Jan Schmidt 2008-01-17 17:00:57 UTC
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?
Comment 7 Tim-Philipp Müller 2008-01-17 17:08:16 UTC
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.
Comment 8 Jan Schmidt 2008-01-17 21:33:40 UTC
boffo
Comment 9 Tim-Philipp Müller 2008-01-17 22:23:19 UTC
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.

Comment 10 Tim-Philipp Müller 2008-01-19 21:26:08 UTC
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.
Comment 11 Tim-Philipp Müller 2008-01-19 21:38:35 UTC
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
Comment 12 David Schleef 2008-01-19 21:50:51 UTC
Thinking long-term, we should add a new, sane replacement for g_thread_init() to glib.
Comment 13 Jan Schmidt 2008-01-19 23:22:08 UTC
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.
Comment 14 Tim-Philipp Müller 2008-01-20 10:49:22 UTC
> 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.
Comment 15 Jan Schmidt 2008-01-20 11:52:05 UTC
(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.
> 

Comment 16 Tim-Philipp Müller 2008-01-20 12:18:15 UTC
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.
Comment 17 Tim-Philipp Müller 2008-01-20 15:05:59 UTC
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.