Created attachment 112709 [details] [review]
remove the ts0 special handling

We have this there to return something sane if no control point is given: the default value. Otherwise, what would you return if there's no control point but a value is requested?

The behaviour currently is, to always have a control point at zero (what would you do if you have a cp at 1 and 0 is requested?) which is the default value of the property if nothing is set. I don't see a problem with that ;)

The problem is that we don't add one for ts=0 by default. So the problem is if I add some and want to dettach the controller when I have removed that last one, I will never dettach it as
gst_interpolation_control_source_get_count() will never return 0

on the other hand _unset_all() will remove all control points and not keep one for ts=0.

Then something is broken ;) What would you suggest for fixing it? I don't like your patch for the above reasons...

We should somehow make sure that there's always the default value for ts=0, otherwise the interpolation functions can't produce a value for ts>0 and ts<smallest cp.

Yes, I don't like the patch either. I just wanted to check whats the reason for the ts=0 handling. Please note, that even with my patch applied the test suite passes.

So what should we do:

1.) Add more tests
2.) when creating an instance for an interpolation control source add cp@0 with default value and set a flag default_ts0=TRUE
3.) it one sets a cp@0, do default_ts0=FALSE
4.) if one usets a cp@0, do
if (!default_ts0) {
  set to default
  nvalues--;
}

If you agree, I'll make new patch.

Sounds good I guess

Hmm, still not sure if we need the complexity. What about:

 static inline GValue *
 gst_interpolation_control_source_get_first_value (GstInterpolationControlSource
     * self)
 {
   if (self->priv->values && self->priv->nvalues > 0) {
     GstControlPoint *cp = self->priv->values->data;

     return &cp->value;
   } else {
-     return NULL;
+     return &self->default_value;
   }
 }

together with my previous patch?

Created attachment 112792 [details] [review]
improve the ts=0 special handling

I've now moved the special case handling to _interpolate_linear_get_##vtype (same changes needed for cubic).
Once the core freeze is over I will first commit a bugfix thats in the (overflow in slope calculation for unsigned value types) and update this patch.

I wonder also if we really need gst_interpolation_control_source_get_first_value(). There is three uses left. One goes away with updating cubic and the other could use direct code (that uses the default value) too.

Created attachment 112793 [details] [review]
two more tests

First tests checks handling of getting values before first control point.

Second tests checks that control count goes down to 0 after removing control points one by one.

Looks good, please update cubic too and remove _get_first_value() if possible :)

2008-06-20  Stefan Kost  <ensonic@users.sf.net>

	* libs/gst/controller/gstinterpolation.c:
	* libs/gst/controller/gstinterpolationcontrolsource.c:
	* tests/check/libs/controller.c:
	  Rewrite handling of default values. Fix overflow with unsigned types
	  in linear interpolation. Remove now obsolete _first_value() function.
	  Add more tests. Fixes #538201.