[Bug 743248] Add new control binding GstAppControlBinding

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=743248
  GStreamer | gstreamer (core) | unspecified

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #295004|none                        |needs-work
             status|                            |

--- Comment #1 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2015-01-20 16:00:07 UTC ---
Review of attachment 295004:
 --> (https://bugzilla.gnome.org/review?bug=743248&attachment=295004)

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

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.