GNOME Bugzilla – Bug 743248
Add new control binding GstAppControlBinding
Last modified: 2018-11-03 12:24:57 UTC
Created attachment 295004 [details] [review] patch GstAppControlBinding is a new GstControlBinding subclass that emits a signal ('update-value') in order to update the value of its bound property. This allows applications to connect to this signal and to set controllable properties along the time-line, without being tied to GstControlSource / GstDirectControlBinding. A patch of a first implementation, including basic unit tests and documentation is provided. Example use case: A live mixing application uses compositor/glvideomixer to animate multiple video layers along an application-defined time-line. This is done by setting the properties xpos/ypos/width/height of compositor's sink pads accordingly. Using GstControlSource / GstDirectControlBinding doesn't work here because: -) 0..1 sources would be mapped to the full property range. That doesn't really make sense for properties using pixel coordinates (e.g. xpos/ypos/width/height). -) Since this a *live* mixing application, changes of a video layer's properties (i.e. sink pad's xpos/ypos/width/height properties) need to have immediate effect (e.g. animating a transition based on real-time user input). One could try to just set xpos/ypos/width/height using a single g_object_set call, but since this is not guaranteed to be atomic, this could result in one frame to already have updated xpos/ypos values, but not yet updated width/height values, resulting in undesired animation artefacts. Using GstAppControlBinding, the application simply binds to xpos/ypos/width/height, which results in the signal being emitted before each rendered frame for all bound properties. The application could then make sure that the right xpos/ypos/width/height tuple is used for the requested render time. Further remarks: (1) Since GstAppControlBinding does have to map GstControlSource's 0..1 value to something useful (like GstDirectControlBinding does), we could support further PARAM_SPEC types (like strings, chars, whatever). However, until a good use case for this comes up, GstAppControlBinding only supports the same types as GstDirectControlBinding. (2) The performance of the installed signal ('update-value') might be improved by passing more advanced flags to g_signal_new (accumulators, etc). Out of my lacking experience with this, I have only installed a basic signal for now. (3) If GstAppControlBinding is asked for an array of values, it is currently emitting multiple signals, for each time-sample of the requested array, in order to fill the array. This could be optimized by using another signal that returns an array of GValues instead of just a single GValue. (4) The GValue instance returned by the 'update-value' signal is currently expected to hold the same type as the bound property. Alternatively, type conversion could be attempted, if supported. (5) Similar to GstDirectControlBinding, GstAppControlBinding does not set the target property, if the value hasn't changed. (6) I tried to extend the documentation to document GstAppControlBinding, I hope I haven't missed anything :) (7) Credits: I have added Stefan Sauer (ensonic) to the license headers, because most of the code was based on his GstDirectControlBinding implementation, and I have also added Sebastian Dröge (slomo), because GstAppControlBinding was his idea. I am looking forward to your feedback!
Review of attachment 295004 [details] [review]: Looks mostly good to me, just some minor comments :) ::: libs/gst/controller/gstappcontrolbinding.c @@ +90,3 @@ +DEFINE_STORE_G_VALUE (float); +DEFINE_STORE_G_VALUE (double); +DEFINE_STORE_G_VALUE (boolean); Hmm, I think for this we could use something from GTypeValueTable. That should be lcopy_value, which is used e.g. by g_object_get(). @@ +122,3 @@ +DEFINE_ARE_EQUAL (float); +DEFINE_ARE_EQUAL (double); +DEFINE_ARE_EQUAL (boolean); You could use gst_value_compare() here @@ +169,3 @@ + */ + signals[SIGNAL_UPDATE_APP_VALUE] = + g_signal_new ("update-value", G_TYPE_FROM_CLASS (klass), Signal enum and property name are out of sync :) @@ +310,3 @@ + if (G_LIKELY (app_val)) { + + // Always update the first sync, then only if value changed No C99 comments :) @@ +317,3 @@ + + if (!self->last_value) { + empty newline @@ +336,3 @@ + + return TRUE; + and here @@ +434,3 @@ + if (app_val) { + g_value_init (&values[i], type); + g_value_copy (app_val, &values[i]); Why do you copy here instead of just memcpy(&values[i], app_val, sizeof (GValue)); g_free(app_val);? @@ +499,3 @@ + + // TODO: could allow app to return anything that is convertible to the + // expected type and try to convert here. For the moment, we are strict C99 comment ::: libs/gst/controller/gstappcontrolbinding.h @@ +84,3 @@ + GValue * last_value; + GstAppControlBindingStoreValue store_func; + GstAppControlBindingValuesAreEqual equality_func; I think these should all go into instance private data, together with their function pointer typdefs
Also as discussed on IRC, it might make sense to add a callback mode like in appsrc/appsink :) That should probably also already have a variant for getting an array though, with NULL going into the fallback code. Adding that later might be tricky in a backwards compatible way.
Stupid question: wouldn't it be much easier to simply add a signal/callback that gets fired from gst_object_sync_values() directly, instead of going through a control binding? (Which has the added value that you don't need one per property, but can just set multiple properties in one go). And much less code.
Review of attachment 295004 [details] [review]: ::: libs/gst/controller/gstappcontrolbinding.c @@ +28,3 @@ + * A binding object that emits a signal to update the bound value (see + * 'update-value' signal of GstAppControlBinding). + */ Can you make it clearer when this controlbinding should be used: - mention that it is to be used without a control-source - mention that is can be used to to atomic property changes of a set of properties
Thanks for your analysis. As a point for discussion I totally agree that the behaviour of GstDirectControlBinding is not perfect form some elements, w.r.t mapping the 0...1 to the property range. We should consider solving this with a new GstScaledControlBinding. This will work the same as the DirectControlBinding but take two extra GValue for the custom lower and upper bound: GstControlBinding * gst_scaled_control_binding_new (GstObject * object, const gchar * property_name, GValue *min_value, GValue *max_value, GstControlSource * cs); The constructor wound need to check that the given min/max are of the same type as the property and are within the actual property bound.
Created attachment 295163 [details] [review] updated GstAppControlBinding patch I have addressed all the issues mentioned above, including - Optionally use callbacks for performance reasons (see gst_app_control_binding_set_callbacks). I also added new unit tests for this. - Split unit tests into sync, get_value, get_value_array, get_g_value_array, as mentioned in IRC One thing that I wasn't quite sure about, is whether it is expected that the binding object is unlocked (GST_OBJECT_UNLOCK) during the callbacks/notify-destroy function or not. As it is now, the object is locked. Please check if I should change this behavior.
I would also to sum up the discussions (partially from IRC) about this bug: (1) Adding a callback/signal in gst_object_sync_values probably solves the problem of setting multiple properties atomically more elegantly w.r.t. the necessary amount of client code because only a single callback is needed for setting multiple properties. This could lead to multiple callbacks per sync, though, coming from different sources. These need to be filtered by the app. A one-shot notification could be useful if multiple properties need to be changed atomically, but only once. (2) GstAppControlBinding implements GstControlBinding, and as such could potentially be reused in any code that uses GstControlBinding already. However, it seems that GstControlBinding is only used by gst_object_sync_value anyway. (3) GstAppControlBinding has the advantage that no existing GStreamer core classes need to be changed, i.e. it only uses already-stable ABI. (4) GstControlBinding / GstControlSource could be redesigned to support a wider range of use cases for live/scheduled property changes in general, and for this bug's use case in particular. g_object_bind_property_full could be useful here. Updating multiple properties at once is something that could also be part of this design (a transaction of value changes, possibly establishing an one-to-many relationship between sources and properties via a new controller design). I can't judge the ramifications of (1), or how useful (2) really is, with my limited experience of the overall GStreamer ecosystem. Because of (3), we could just leave this bug open, with the attached patch for anyone to use, and to work around the issue, until (4) would really nail this issue. But (4) needs much more input and better documentation of use cases. I would be happy to help out here, once I can afford shifting my focus on the requirements of parameter controls, and once I can speak more publicly about my current work (mostly a time-issue than a legal one) :) (summer/spring 2015?).
FYI, regarding the scaling there is also #740502 adding an 'absolute' flag to DirectControlBinding to bypass 0...1 -> min...max mapping.
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #8) > FYI, regarding the scaling there is also #740502 adding an 'absolute' flag > to DirectControlBinding to bypass 0...1 -> min...max mapping. bug 740502 has now been merged.
-- 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/88.