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

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=644776
  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.

Ack.

> #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
GST_ELEMENT_CHECK_PARAM_MUTABILITY() or so.


>   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 |
GST_PARAM_MUTABLE_PLAYING));

  etc.?


> (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
appropriate.

-- 
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.