After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 754678 - controller: the cubic interpolation we have implemented can overshoot min/max of the control-point values
controller: the cubic interpolation we have implemented can overshoot min/max...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.x
Other Linux
: Normal enhancement
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-07 13:39 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2016-01-05 12:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtk demo to try different interpolationmodes (8.32 KB, patch)
2015-09-07 13:40 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review
refactor the interpolation code (9.72 KB, patch)
2015-09-07 13:40 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review
add new monotonic cubic splines (7.47 KB, patch)
2015-09-07 13:40 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
add new monotonic cubic splines (8.07 KB, patch)
2015-09-07 13:45 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review
cubic splines (15.91 KB, image/png)
2015-09-07 13:47 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
  Details
monotonic cubic splines (16.50 KB, image/png)
2015-09-07 13:47 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
  Details

Description Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-07 13:39:08 UTC
I will attach screenshots + patches for a new interpolation mode.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-07 13:40:12 UTC
Created attachment 310816 [details] [review]
gtk demo to try different interpolationmodes
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-07 13:40:34 UTC
Created attachment 310817 [details] [review]
refactor the interpolation code
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-07 13:40:59 UTC
Created attachment 310818 [details] [review]
add new monotonic cubic splines
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-07 13:45:32 UTC
Created attachment 310819 [details] [review]
add new monotonic cubic splines
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-07 13:47:34 UTC
Created attachment 310820 [details]
cubic splines
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-07 13:47:58 UTC
Created attachment 310821 [details]
monotonic cubic splines
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-27 10:47:53 UTC
commit 56f12705cab3affa2fb03fad7e9eb9859f1b9137
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Mon Sep 7 09:39:32 2015 +0200

    interpolationcontrolsource: add cubic_mono interpolation
    
    This new mode won't overshoot the min/max y values set by the control-points.
    Fixes #754678
    API: GST_INTERPOLATION_MODE_CUBIC_MONO

commit 2fe3939ce7ea84c45dd922e7f1097dd07f11fc5d
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Mon Sep 7 09:37:05 2015 +0200

    interpolationcontrolsource: refactor code
    
    Extract common code that looks up the control-points around the timestamp. Add
    some comments for future investigation.

commit 6e7915d5cdc00c5f9d3ad880f5ac96d771bb63de
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Fri Sep 4 16:38:37 2015 +0200

    tests/examples: add a demo for the interpolation control source modes
    
    This is in preparation for new modes to be added. In particullar it demonstrates
    how the cubic splines overshoot the range.
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-28 11:45:46 UTC
Meh, this breaks the libs/libsabi test since I added another double in the union in GstControlPoint. Unfortunately this stuct does not even have padding.

This struct should not have been in a public header in the first place :/ It is not used on any public functions as a parameter.

So what can we do?

a) I privately clone GstControlPoint once more and add a bunch of TODOs
b) I prep a patch that move GstControlPoint to a private header now and remove it from the abi checks.

b) would be an API+ABI break, if there is actually someone peeking into this structure. Any opinions?
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-28 12:31:09 UTC
Meh #2, this struct needs padding. If we need to preserve compat for  now, I'll have to revert the changes and wait for 2.0.

The only solution that keeps the ABI I can think of would be to use a gpointer in the union (smaller than two doubles) and then in the GstInterpolationControlSource keep a GHashTable<gpointer, struct-with-3-doubles>. The struct-with-3-doubles would be allocates on the fly. The hashtables would be nuked on finalize.
Comment 10 Tim-Philipp Müller 2015-09-28 12:40:29 UTC
I think you should take a common-sense and risk-based approach to this: do you know if any external code is using this structure? is it likely anyone is using this?

As far as I can see the only reason to use this is for subclasses to peek into the GSequence directly or modify it? What are the chances of anyone doing that?

It's not directly used in any API, so I think you have some leeway here even if strictly speaking it would be an ABI break.

Question is also who allocates these structs? It's not like they'll be used on the stack. So I guess the main risk is some <=1.6 code allocating such a struct which would be too small then and being used in combination with libgstcontroller >= 1.7 which expects the struct to be larger.
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2015-09-28 14:37:52 UTC
commit d4f81fb4e62d34a4c1dabc65b23ede7ce7694c63
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Mon Sep 28 16:19:40 2015 +0200

    controller: add the missing abi padding
    
    While this technically is an abi break, we decided to do this:
    1) the struct is documented to be internal
    2) the struct is alloced and freed inside the library
    3) there are no public methods that receive or return instances
    4) the only code known to use this struct are classes containd here
Comment 12 Tim-Philipp Müller 2016-01-03 19:52:53 UTC
Just came across this when writing release notes.

Is there any particular reason this was named _MONO rather than _MONOTONIC? Is 'mono' a common abbreviation for 'monotonic' used elsewhere? I think in general _MONOTONIC would be better, even if it's a bit longer.
Comment 13 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-03 22:38:40 UTC
Yes _MONOTONIC is more precise and _MONO is probably not a common abbreviation in this context. The enum names are getting close to the 80 column limits though (especially when used with the api).
GST_INTERPOLATION_MODE_CUBIC_MONO{,TONIC}

Do you want me to rename everything?
Comment 14 Sebastian Dröge (slomo) 2016-01-04 07:45:51 UTC
Renaming to MONOTONIC seems like a good idea
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-05 12:43:27 UTC
Okay, let me rename it ...
Comment 16 Stefan Sauer (gstreamer, gtkdoc dev) 2016-01-05 12:59:22 UTC
commit ab17881cf0e77c36098a5dd873e3b64d135e1eb1
Author: Stefan Sauer <ensonic@users.sf.net>
Date:   Tue Jan 5 13:57:12 2016 +0100

    controller: rename new cubic interpolation mode
    
    Don't abbreviate to 'mono' and use 'monotonic' instead.