[Bug 644776] Macro to check check mutability in set_property functions

GStreamer (bugzilla.gnome.org) bugzilla at gnome.org
Thu Mar 17 17:09:19 PDT 2011

  GStreamer | gstreamer (core) | git

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
             Status|UNCONFIRMED                 |NEW
                 CC|                            |t.i.m at zen.co.uk
     Ever Confirmed|0                           |1

--- Comment #1 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2011-03-18 00:09:15 UTC ---
> Now that we have GST_PARAM_MUTABLE_*, we should add a way to enforce them.

I'm sure we had a bug for this already somewhere. In any case, I agree that a
convenience function or macro for this would be nice. Not sure *enforce* is the
right word though, since these g_return_if_fail() may not be around on e.g.
embedded systems, if GStreamer was compiled with the right -DG_DISABLE_*.

> I suggest having a macro like the following and putting it as the first line of
> the set_property() function in the various elements after adding the mutability
> flags.


> #define GST_PARAM_CHECK_MUTABILITY(element, pspec) G_STMT_START {       \

While it does probably belong into the GstParam section in the API reference,
the first argument is a GstElement, so arguably it should be

>   g_return_if_fail ((element) != NULL && GST_IS_ELEMENT (element));     \

This seems pointless in a set_property function which is looked up from the
class (ie. if element was NULL or not a GstElement, we'd never get to said
set_property function in a GstElement subclass.).

>   GstState state;                                                       \
>                                                                         \
>   GST_OBJECT_LOCK (element);                                            \
>   state = GST_STATE (element);                                          \
>   GST_OBJECT_UNLOCK (element);                                          \

This is just for sanity checking purposes, so I think maybe a
GST_STATE(element) without locking might just do. We probably don't want to
lock more than needed

>   g_return_if_fail (state == GST_STATE_PLAYING ||                       \
>       (pspec)->flags & GST_PARAM_MUTABLE_PLAYING);                      \
>   g_return_if_fail (state >= GST_STATE_PAUSED ||                        \
>       (pspec)->flags & GST_PARAM_MUTABLE_PAUSED);                       \
>   g_return_if_fail (state >= GST_STATE_READY ||                         \
>       (pspec)->flags & GST_PARAM_MUTABLE_READY);                        \

Is the logic right here? shouldn't it be:

  g_return_if_fail (state < GST_STATE_PLAYING ||
                          (pspec)->flags & GST_PARAM_MUTABLE_PLAYING);

  g_return_if_fail (state < GST_STATE_PAUSED ||
                          (pspec)->flags & (GST_PARAM_MUTABLE_PAUSED |


> (and maybe we  want to use GST_ERROR_OBJECT() instead
> of g_warning .. maybe g_critical?

g_warnings seems fine here IMHO. This is most likely a programming mistake by
the application developer after all, for which g_return_if() and friends are

Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.

More information about the gstreamer-bugs mailing list