GNOME Bugzilla – Bug 774657
add proxy control binding
Last modified: 2016-11-27 14:56:59 UTC
See commit message.
Created attachment 340208 [details] [review] controllers: add new proxy control binding
Review of attachment 340208 [details] [review]: Generally makes sense, but: ::: libs/gst/controller/gstproxycontrolbinding.c @@ +167,3 @@ + "name", property_name, NULL); + + cb->ref_object = ref_object; Should probably store a new reference? @@ +168,3 @@ + + cb->ref_object = ref_object; + cb->property_name = ref_property_name; ... and g_strdup() this one?
Created attachment 340488 [details] [review] controllers: add new proxy control binding - Fix some valgrind warnings - g_strdup property names - use GWeakRef for the referenced object so that can disappear.
Comment on attachment 340488 [details] [review] controllers: add new proxy control binding Pushed after adding new symbols to .def files. commit 3eb48964358149f9934656f681402a431b262e5f Author: Matthew Waters <matthew@centricular.com> Date: Fri Nov 18 13:09:21 2016 +1100 controllers: add new proxy control binding Allows proxying the control interface from one property on one GstObject to another property (of the same type) in another GstObject. E.g. in a parent-child relationship, one may need to gst_object_sync_values() on the child and have a binding (set elsewhere) on the parent update the value. Note: that this doesn't solve GObject property forwarding and must be taken care of by the implementation manually or using GBinding. https://bugzilla.gnome.org/show_bug.cgi?id=774657
I saw this a bit late, but please improve the doc blob. It should tell what is this good for. " A #GstControlBinding that forwards requests to another #GstControlBinding. " is not giving people enough to understand in what situations this is useful. TBH, I also don't understand the commit message " E.g. in a parent-child relationship, one may need to gst_object_sync_values() on the child and have a binding (set elsewhere) on the parent update the value. " Do you have a concrete example?
Oh and thanks for adding a test, but please break this down into multiple tests. If the tests fails, one just know that *something* called controller_proxy is broken. Having several tests allows you to tell what you test in the test name.
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #5) > I saw this a bit late, but please improve the doc blob. It should tell what > is this good for. > > " > A #GstControlBinding that forwards requests to another #GstControlBinding. > " > > is not giving people enough to understand in what situations this is useful. For exactly ^. You have two objects. You need to forward the control binding from one property on one object to another property on another object. > TBH, I also don't understand the commit message > > " > E.g. in a parent-child relationship, one may need to > gst_object_sync_values() on the child and have a binding (set elsewhere) > on the parent update the value. > " > > Do you have a concrete example? glvideomixer/glsinkbin. sync_values() on the child pad/element will call sync_values on the exposed bin pad/element. (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #6) > Oh and thanks for adding a test, but please break this down into multiple > tests. If the tests fails, one just know that *something* called > controller_proxy is broken. Having several tests allows you to tell what you > test in the test name. How would I break this down? All the implementation does is forward to the necessary control binding. There isn't really a logical split of functionality to test.
(In reply to Matthew Waters (ystreet00) from comment #7) > (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #5) > > I saw this a bit late, but please improve the doc blob. It should tell what > > is this good for. > > > > " > > A #GstControlBinding that forwards requests to another #GstControlBinding. > > " > > > > is not giving people enough to understand in what situations this is useful. > > For exactly ^. > > You have two objects. You need to forward the control binding from one > property on one object to another property on another object. > > > TBH, I also don't understand the commit message > > > > " > > E.g. in a parent-child relationship, one may need to > > gst_object_sync_values() on the child and have a binding (set elsewhere) > > on the parent update the value. > > " > > > > Do you have a concrete example? > > glvideomixer/glsinkbin. sync_values() on the child pad/element will call > sync_values on the exposed bin pad/element. > So far I did this in the parent (calling gst_element_sync_values on the children). But in your case it seems to be that child and parent also expose the same property, right? > > (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #6) > > Oh and thanks for adding a test, but please break this down into multiple > > tests. If the tests fails, one just know that *something* called > > controller_proxy is broken. Having several tests allows you to tell what you > > test in the test name. > > How would I break this down? > > All the implementation does is forward to the necessary control binding. > There isn't really a logical split of functionality to test. I disagree, I'll post review comment now.
Review of attachment 340488 [details] [review]: This is really about test maintenance. I did not invent these best practices and I recently sent a mail to the list about it ("better unit tests" - http://gstreamer-devel.966125.n4.nabble.com/better-unit-tests-td4680069.html) ::: tests/check/libs/controller.c @@ +1545,3 @@ + cb = gst_proxy_control_binding_new (GST_OBJECT (elem), "int", + GST_OBJECT (elem2), "int"); + GValue gval1 = G_VALUE_INIT, gval2 = G_VALUE_INIT; personally I don't like asserts that are out side of the scope of the test. We already have a test that verifies that we can add_control_bindings, right? @@ +1551,3 @@ + fail_unless (val1 == NULL); + fail_if (gst_control_binding_get_value_array (cb, 0, 0, 1, &int1)); + elem = gst_element_factory_make ("testobj", NULL); here the first test ends. It could be called 'controller_proxy_does_nothing' @@ +1558,3 @@ + + cb2 = gst_direct_control_binding_new (GST_OBJECT (elem2), "int", cs); + /* proxy control binding from elem to elem2 */ see above. In general a test should ideally have a single assert (or fail_unless/if). This single check is what the tests verifies. @@ +1589,3 @@ + g_free (val2); + g_value_unset (&gval1); + fail_unless (gst_object_add_control_binding (GST_OBJECT (elem2), cb2)); Here you verify 3 things. You tests that 3 different functions are proxied: get_value, get_value_array and get_g_value_array. This could be 3 tests. If you feel it would repeat code, use helpers. @@ +1615,3 @@ + g_free (val2); + g_value_unset (&gval1); + g_value_get_int (val1)); This repeats the above code. This can be done more efficiently using a 'loop' test: https://libcheck.github.io/check/doc/check_html/check_4.html#Looping-Tests @@ +1624,3 @@ + time = 1 * GST_SECOND; + gst_object_sync_values (GST_OBJECT (elem2), time); + fail_unless (gst_control_binding_get_value_array (cb2, time, 0, 1, &int2)); Another test: e.g. 'controller_proxy_sync_original_still_work', should also be a loop test.