Created attachment 128600 [details] [review]
patch

Looks good and sounds like a good idea to add this :)
But you could simply check for <= _PAUSED, etc. instead of all the different comparisons. Also you should probably handle GST_STATE_VOID_PENDING, which is even smaller than _NULL ;)

Not a bad idea at all, but wouldn't it make just as much sense to make all properties that can be manipulated while the pipeline is running controllable? That seems more user-friendly to me, since the user is likely to care only about 'pipeline is in NULL state' and 'pipeline is running', but not about the technical details of the states in between. (They wouldn't *have* to use the GstController API to change the property of course).

Also, some of our state stuff is just inconsistent and unintuitive, e.g. why wouldn't filesrc open the device in NULL->READY? It's not really READY if the device isn't open yet, is it? But other devices are opened in NULL->READY. Do we really want to expose all of this even more?

(Not against this at all, just thinking out loud)

CONTROLLABLE is a bit different IMHO. You can change some properties while PLAYING but it is quite expensive (all the properties in audiowsinclimit for example) so you usually don't want to change them every 20 milliseconds and don't want them controlled by a GstController.

IMHO this difference between CONTROLLABLE and MUTABLE_PLAYING is important ;)

Fair enough :)

The docs for gst_param_spec_is_mutable() should advice which lock the application needs to take to avoide a race.

Btw. there are still the flags that I proposed in Bug #522205. 

What's happening with this one? Let's commit it and get it into 0.10.23 if everyone's happy with the API.

There are some comments above that could go into the patch.

> The docs for gst_param_spec_is_mutable() should advice which lock the
> application needs to take to avoide a race.

Stefan, please add what you think is appropriate to the documentation after this is pushed.

commit 1ecf114c0ecb7477cbdf9f8b5c772d2e93489064
Author: David Schleef <ds@schleef.org>
Date:   Fri Feb 20 11:09:19 2009 -0800

    Add param spec flags for when a property can be changed
    
    Adds GST_PARAM_MUTABLE* flags to indicate in which states a
    property can be changed and take effect.  Fixes #571559

I am not sure what the solution is. Lets imagine one has this code in the app:

g_object_set(element, "param", value, NULL);

Now, one should do:

GParamSpec *pspec;
GstElementClass element_class;

element_class = GST_ELEMENT_GET_CLASS (element);
pspec = g_object_class_find_property(element_class, "param");
if (gst_param_spec_is_mutable (pspec, element)) {
  g_object_set(element, "param", value, NULL);
}

This is ofcourse racy, as one needs to take a lock. The gst_param_spec_is_mutable() already takes GST_OBJECT_LOCK(), so the app can't use that. Should we rather have 
gboolean gst_param_spec_set_if_mutable(element_class, "param", value);
?

Also we should add another API
gst_element_property_is_mutable (element, property_name)
for convinience.

I understand now.  gst_param_spec_is_mutable() should be changed to have locked and unlocked versions.  The function was not meant to be used by applications.  I'm not sure it would be useful in applications, since applications should be checking the flags directly.

Out of curiosity whats the use case you where thinking about? 

One non-application use case I will do, is to make binding a controller to a object property that can't change in playing fail. There I should access the flags directly.

Inside set_property() functions in plugins, as in the filesrc part of the patch above.

I suppose gst_param_spec_is_mutable() should not take the lock, since it should be called with the lock held inside the critical section of set_property() functions.  Fixing.

commit 8dd2b4b591b7650cfabad2ffc79af7f20d673012
Author: David Schleef <ds@schleef.org>
Date:   Tue Apr 14 12:20:37 2009 -0700

    Fix locking in gst_param_spec_is_mutable


Keep in mind that these flags are only advisory right now, so binding a controller should only fail if MUTABLE_PAUSED or MUTABLE_READY is set, but not MUTABLE_PLAYING.  If none are set, the old behavior (that is, "fail randomly") should remain.

Somebody please save us from GObject properties in 0.11.

It's still very tricky to use, you basically force elements to use the object lock to protect their properties, which is, more often than not, impossible.