[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
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.
More information about the gstreamer-bugs
mailing list