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

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

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 ?

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.

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.

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.

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

Created attachment 339704 [details] [review]
element: add GST_ELEMENT_CHECK_PROPERTY_SET_MUTABILITY

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.

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.

Review of attachment 339705 [details] [review]:

Looks good

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