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 644776 - Macro to check check mutability in set_property functions
Macro to check check mutability in set_property functions
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-03-14 23:41 UTC by Olivier Crête
Modified: 2018-11-03 12:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
paramspecs: Add function and macro to check parameter mutability. (3.94 KB, patch)
2013-02-24 02:31 UTC, Olivier Crête
needs-work Details | Review
element: add GST_ELEMENT_CHECK_PROPERTY_SET_MUTABILITY (4.35 KB, patch)
2016-11-12 18:18 UTC, Tim-Philipp Müller
reviewed Details | Review
elements: use new property mutability check macro (12.27 KB, patch)
2016-11-12 18:21 UTC, Tim-Philipp Müller
accepted-commit_now Details | Review

Description Olivier Crête 2011-03-14 23:41:40 UTC
Now that we have GST_PARAM_MUTABLE_*, we should add a way to enforce them.

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 {       \
  GstState state;                                                       \
                                                                        \
  g_return_if_fail ((element) != NULL && GST_IS_ELEMENT (element));     \
  GST_OBJECT_LOCK (element);                                            \
  state = GST_STATE (element);                                          \
  GST_OBJECT_UNLOCK (element);                                          \
  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);                        \
  } G_STMT_END

We may want to have a version that doesn't release the object lock if the paramspec is mutable above NULL, since I guess for many cases, we'd want to make the change atomic, but that means being a little more clever (and maybe we want to use GST_ERROR_OBJECT() instead of g_warning .. maybe g_critical?
Comment 1 Tim-Philipp Müller 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.
Comment 2 Sebastian Dröge (slomo) 2011-03-18 08:19:21 UTC
That's bug #571559 , which added the mutable flags. The patch had a function to check mutability but that part was dropped because of several reason that apply to this macro here too:
* It's not threadsafe if you don't lock or release the lock after checking and before doing things
* Using the object lock here requires the element to protect their properties with the object lock and only with the object lock
Comment 3 Olivier Crête 2011-03-18 17:39:04 UTC
I had completely missed that bit of the patch of bug #571559.

I'm not sure if we want t completely disable that check if compiled with G_DISABLE_CHECKS.. If we do, a better approach then is to use the function from the other bug and do something like this in the various functions:

g_return_if_fail (gst_param_spec_is_mutable (pspec, element));

That said, this doesn't help for the case where we'd like to keep the lock held after doing the check to prevent a state change. Maybe we want a _locked() version ?
Comment 4 Olivier Crête 2013-02-24 02:31:58 UTC
Created attachment 237260 [details] [review]
paramspecs: Add function and macro to check parameter mutability.

Updated based on the comments here and on #571559. I'm proposing a function that doesn't do any locking as well
as a macro that takes the object lock, as I think it's the most common use-case. That said, even if the object does not
use the object lock to protect its data, it must still take it to make sure that no state transition happens while
the property is being set.
Comment 5 Tim-Philipp Müller 2013-02-24 12:40:03 UTC
Ok, so, question: what are we trying to do here exactly?

Are we trying to

a) protect the element from dealing with property changes that shouldn't be happening?

b) catch programming errors and alert the programmer?

In case a) we need to worry about locking etc. etc., while in case b) we probably don't.
Comment 6 Sebastian Dröge (slomo) 2013-07-24 08:36:00 UTC
I think we only want b) here, it's a programming error and nothing else. Elements should make sure themselves that nothing bad happens if properties are wrongly set.
Comment 7 Sebastian Dröge (slomo) 2013-07-24 08:38:05 UTC
Review of attachment 237260 [details] [review]:

This is still racy btw, I think we should just omit the locking and catch programming errors... so do a g_warning() or something like that
Comment 8 Tim-Philipp Müller 2016-11-12 18:18:00 UTC
Created attachment 339704 [details] [review]
element: add GST_ELEMENT_CHECK_PROPERTY_SET_MUTABILITY
Comment 9 Tim-Philipp Müller 2016-11-12 18:21:02 UTC
Created attachment 339705 [details] [review]
elements: use new property mutability check macro

Seems to work, judging by the 'make check' carnage:

bin:test_state_change_skip: Trying to set property state-error on element fakesink in READY state, but property is not marked as mutable in ready state.

gst/gstelement:test_property_notify_message: Trying to set property dump on element identity0 in READY state, but property is not marked as mutable in ready state.

identity:test_signal_handoffs: Trying to set property signal-handoffs on element identity0 in PLAYING state, but property is not marked as mutable in paused or playing state.

multiqueue:test_watermark_and_fill_level: Trying to set property low-watermark on element multiqueue0 in PAUSED state, but property is not marked as mutable in paused or playing state.
multiqueue:test_high_threshold_change: Trying to set property high-percent on element multiqueue0 in PAUSED state, but property is not marked as mutable in paused or playing state.
multiqueue:test_low_threshold_change: Trying to set property high-percent on element multiqueue0 in PAUSED state, but property is not marked as mutable in paused or playing state.

selector:test_output_selector_no_srcpad_negotiation: Trying to set property pad-negotiation-mode on element outputselector0 in PLAYING state, but property is not marked as mutable in paused or playing state.

tee:test_allow_not_linked: Trying to set property allow-not-linked on element tee in PLAYING state, but property is not marked as mutable in paused or playing state.

queue2: test_watermark_and_fill_level: Trying to set property low-watermark on element queue2-0 in PAUSED state, but property is not marked as mutable in paused or playing state.
Comment 10 Sebastian Dröge (slomo) 2016-11-14 09:13:33 UTC
Review of attachment 339704 [details] [review]:

Looks good but

::: gst/gstelement.h
@@ +645,3 @@
+
+/**
+ * GST_ELEMENT_CHECK_PROPERTY_SET_MUTABILITY_UNLOCKED:

Elsewhere in our code, the unlocked variants are the ones that assume outside to take the lock. And the non-unlocked variants are the ones that take the lock before calling the unlocked variant.
Comment 11 Sebastian Dröge (slomo) 2016-11-14 09:14:45 UTC
Review of attachment 339705 [details] [review]:

Looks good
Comment 12 GStreamer system administrator 2018-11-03 12:14:46 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/19.