Created attachment 337838 [details] [review]
paramspecs: Add int range.

Review of attachment 337838 [details] [review]:

Generally makes sense (maybe do the same for the other ranges and use a macro to generate code?) but:

::: gst/gstparamspecs.c
@@ +240,3 @@
+  g_return_val_if_fail (gst_value_get_int_range_step (value) > 0, TRUE);
+
+  if (gst_value_get_int_range_max (value) < gst_value_get_int_range_min (value)) {

If max < min, all is ok? Shouldn't it be the other way around... and don't you want to check here if min <= value <= max here anyway?

@@ +242,3 @@
+  if (gst_value_get_int_range_max (value) < gst_value_get_int_range_min (value)) {
+    gst_value_set_int_range (value, gst_value_get_int_range_min (value),
+        gst_value_get_int_range_min (value));

And what's the point of this? :)

Stian?

I think we need to clear up how the validate function is supposed to work.

From the docs of GParamSpecTypeInfo::value_validate:
"Ensures that the contents of @value comply with the specifications set out by this type (optional), see g_param_value_validate()".

From the other implementations (e.g param_int_validate(), except _gst_param_fraction_validate()) the value will be clamped to a valid value, and it will return TRUE if the value had to be changed, FALSE if the value was already valid.

So the intent for _gst_param_int_range_validate is to modify value contents in the least destructive way, so that it complies with the requirements. In this case I think that would be to make sure the value's range lies within the allowed range.

The patch does not do this correctly, and I will fix that if we agree that this is the desired behavior?

I'm sure a common macro can be defined for int_range, int64_range and double_range, but probably not fraction_range. And it's not like the macro will make the code any easier to read. I'll wait with that until at least we've agreed on how int_range should be implemented.

The validate func is called when _set_property() is called with a value, to make sure the value in the GValue adheres to the restrictions imposed by the param spec.

 
> So the intent for _gst_param_int_range_validate is to modify value contents
> in the least destructive way, so that it complies with the requirements. In
> this case I think that would be to make sure the value's range lies within
> the allowed range.

Yes, that's how it should be aiui.

 
> The patch does not do this correctly, and I will fix that if we agree that
> this is the desired behavior?
> 
> I'm sure a common macro can be defined for int_range, int64_range and
> double_range, but probably not fraction_range. And it's not like the macro
> will make the code any easier to read. I'll wait with that until at least
> we've agreed on how int_range should be implemented.

I'm all in favour of clarity. Can always macro-fy it later.

Created attachment 364276 [details] [review]
follow-up patch on top of the other patch

Fixed up misc things, mostly the value validate function and the param_spec_new_function (validate with guards).

I've also changed the variable names a bit to 'start' and 'stop' because all the min_min, min_max, max_min, max_max did my head in.

One more thing: I was wondering if we also needed a step_step, so you can say "step can be anything between 16 and 4096 but only in multiples of 16" or have I just confused myself now?

That boils down to 16 as a step no? Wich use case is not covered ?

This is a property spec describing a property of type int range with a start, stop and a step.

For start we have min_start, max_start and default_start.

For stop we have min_stop, max_stop and default_stop.

For step we have min_step, max_step and default_step.

An actual range will have [start, stop, step].

As far as I can tell the pspec can currently only express a range for the allowed step values but not that the step itself can only be a multiple of N rather than anything within min_step and max_step.

(Also, thinking about this some more I think the validation wrt to checking min/max for the step is not right.)

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