[Bug 665294] [0.11] gstvalue: add stepped ranges

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #5 from Sebastian Dröge <slomo at circular-chaos.org> 2011-12-01 15:52:14 UTC ---
Review of attachment 202526:
 --> (https://bugzilla.gnome.org/review?bug=665294&attachment=202526)

Everything I write about int ranges applies to int64 ranges too of course

::: gst/gstvalue.c
@@ +815,3 @@
+  if (vals == NULL) {
+    gst_value_init_int_range (dest_value);
+    vals = (gint *) dest_value->data[0].v_pointer;

You don't use "vals" anywhere

@@ +828,3 @@
     GTypeCValue * collect_values, guint collect_flags)
 {
+  gint *vals = value->data[0].v_pointer;

You don't use "vals" anywhere

@@ +835,3 @@
+  if (n_collect_values > 3)
+    return g_strdup_printf ("too many value locations for `%s' passed",
+        G_VALUE_TYPE_NAME (value));

This does not work, you have to explicitly specify the number of collect values
when registering the type (the ii at the bottom should be an iii). You always
need 3 arguments now and all code has to be change :(

@@ +850,2 @@
+  INT_RANGE_MIN (value) = collect_values[0].v_int;
+  INT_RANGE_MAX (value) = collect_values[1].v_int;

If you store min/max divided by step below in the setter you should do that
here too

@@ +2869,3 @@
+  if (INT_RANGE_MIN (src2) * INT_RANGE_STEP (src2) <= src1->data[0].v_int &&
+      INT_RANGE_MAX (src2) * INT_RANGE_STEP (src2) >= src1->data[0].v_int &&
+      src1->data[0].v_int % INT_RANGE_STEP (src2) == 0) {

Doesn't this ignore the union of, e.g., [1, 3, 1] and 4 (which would be [1, 4,
1])?

@@ +2953,3 @@
+
+  /* TODO: a union which can fail is going to be pretty confusing, no ?
+     Maybe we should just return a list of both values here ? */

No, this is more confusing and this is usually only used in caps... where a
failing union of a value does not result in a failing union, just in more
structures. IMHO we could make all this API here private anyway (and IMHO we
can remove subset() and union() because nothing uses them)

@@ +2982,3 @@
   gint min;
   gint max;
+  gint step;

Wouldn't it be faster to first can_intersect() here?

@@ +2988,3 @@
+      INT_RANGE_STEP (src1) /
+      gst_util_greatest_common_divisor (INT_RANGE_STEP (src1),
+      INT_RANGE_STEP (src2)) * INT_RANGE_STEP (src2);

The multiplication of the two steps here could overflow

@@ +2992,3 @@
+  min =
+      MAX (INT_RANGE_MIN (src1) * INT_RANGE_STEP (src1),
+      INT_RANGE_MIN (src2) * INT_RANGE_STEP (src2));

And here too

@@ +3403,2 @@
+  if (step1 != step2) {
+    /* ENOIMPL */

EPLEASEIMPL ;)

@@ +5263,3 @@
   gst_value_copy_int_range,
   NULL,
   (char *) "ii",

The collect signature is now iii, not ii

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