GNOME Bugzilla – Bug 665294
[0.11] gstvalue: add stepped ranges
Last modified: 2012-01-24 14:21:51 UTC
Preliminary, here to attract comments. It is not possible to represent every result of a set operation using this model, so some return FALSE when this cannot be done, even if the operation is mathematically possible. Another way would be to make lists of ranges, but we decided against this for another case, so...
Created attachment 202525 [details] [review] gstutils: add a 64 bit version of GCD calculation
Created attachment 202526 [details] [review] gstvalue: add stepped ranges int and int64 ranges can now have an optional step (defaulting to 1). Members of the range are those values within the min and max bounds which are a multiple of this step.
Created attachment 202527 [details] [review] tests: add basic tests for new stepped ranges
Review of attachment 202525 [details] [review]: ::: gst/gstutils.c @@ +3535,3 @@ + */ +gint64 +gst_util_greatest_common_divisor_64 (gint64 a, gint64 b) Call it _divisor_int64() to make it more consistent with the scaling functions for example
Review of attachment 202526 [details] [review]: 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
> 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 :( Ah, I see. Well, I moved it to 3, but went back to two arguments because: - it would require changing a lot of client code, also in code I don't even know exists - it turns out there's precious little that has a use for non 1 steps anyway > @@ +2982,3 @@ > gint min; > gint max; > + gint step; > > Wouldn't it be faster to first can_intersect() here? AFAICT, can_intersect will just return TRUE if the types are compatible. Which we already know they are. > @@ +3403,2 @@ > + if (step1 != step2) { > + /* ENOIMPL */ > > EPLEASEIMPL ;) AFAICT, this case was only used for subset determination (A-B and B-A). I've replaced this with an actual subset function (which ought to be faster here, even), so this should not be needed. Especially if we then make this private as you suggested. Rest fixed/changed, plus a bit more tests, and other bugfixes.
Created attachment 202604 [details] [review] gstutils: add a 64 bit version of GCD calculation
Created attachment 202605 [details] [review] gstvalue: add stepped ranges int and int64 ranges can now have an optional step (defaulting to 1). Members of the range are those values within the min and max bounds which are a multiple of this step.
Created attachment 202606 [details] [review] tests: add basic tests for new stepped ranges
(In reply to comment #6) > > 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 :( > > Ah, I see. > Well, I moved it to 3, but went back to two arguments because: > - it would require changing a lot of client code, also in code I don't even > know exists > - it turns out there's precious little that has a use for non 1 steps anyway The only problem I see with this approach is, that it's impossible to specify steps in static pad templates now. Or am I missing something?
> The only problem I see with this approach is, that it's impossible to specify > steps in static pad templates now. Or am I missing something? That'd be unfortunate. I've not tried yet to add stepped ranges to a pad template; the one I knew wanted one was ffmpegcolorspace, but it's not in 0.11. I guess I can try it on any random element for testing though. I'd have thought the text parsing would be used, which does support an optional range.
Ignore what I said, you're right of course
Created attachment 204858 [details] [review] gstvalue: add stepped ranges int and int64 ranges can now have an optional step (defaulting to 1). Members of the range are those values within the min and max bounds which are a multiple of this step.
Now with added functions in the win32 linker list. Still no element using those in their template caps though. If someone knows one that could use it, do tell. Otherwise, ready to push.
Still applies, tests still pass, no further comments... pushed. commit 982ff80c384135934b4696e6d0a35fbb79ea8cb9 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Dec 1 12:43:03 2011 +0000 tests: add basic tests for new stepped ranges https://bugzilla.gnome.org/show_bug.cgi?id=665294 commit 39533f4364b7f3fd8b54f87039acebc705d68c3a Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Nov 30 14:45:12 2011 +0000 gstvalue: add stepped ranges int and int64 ranges can now have an optional step (defaulting to 1). Members of the range are those values within the min and max bounds which are a multiple of this step. https://bugzilla.gnome.org/show_bug.cgi?id=665294 commit 7319b52a2e2236d14c5c85bdb745ced3839309f2 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Nov 30 17:58:07 2011 +0000 gstutils: add a 64 bit version of GCD calculation https://bugzilla.gnome.org/show_bug.cgi?id=665294