From c1c95f538f6e67134bc50f67be6489ecac4ad094 Mon Sep 17 00:00:00 2001 From: Mads Kiilerich Date: Mon, 16 May 2022 17:22:24 +0200 Subject: [PATCH] evoral: fix ControlList::_x_scale to avoid ratio overflow when adjusting fade length Region fades would sometimes get in a mode with weird behaviour. They would be drawn in 2d with crossing lines, mainly moving back and forth horizontally - not as a function of time. It would sound as it looked. The fade would sometimes jump around when resizing. It could be worked around by resetting the fade shape. It turned out the problem could be reproduced by making minute long fades. This change fixes or works around the problem. Back story: timepos_t (in temporal/timeline.h) uses 62 bit for integer value and the max value is thus 4611686018427387904 ~ 5e18. timepos_t counts superclocks, where superclock_ticks_per_second is 56448000 ~ 6e7. It can thus store up to 8e10 seconds - thousands of years. ratio_t (in temporal/types.h) can represent fractions as 64 bit (signed) numerator and denominator. timepos_t avoids floating point operations, but has operator* with ratio_t. To avoid crazy loss of precision it will multiply the superclock count with the numerator before dividing with the denominator. Audio region fade in and out uses a number of increasing timepos_t values (in a ControlList) up to the length of fade. When dragging to resize, these values are (in_x_scale) multiplied with the ratio_t of the new and old fade length. The problem is that the 62 bits will overflow if using fades more than sqrt(5e18) ~ 2e9 superclock ticks ~ 38 seconds. It will overflow into the "beat" flag and (at 58 seconds) into the sign bit. The timepos_t values in the fade will thus jump and can be negative or change to count beats. To work around that problem, this changeset just use floating point values for scaling the timepos_t values. All scaled values are stored as integer anyway, so it should not make any actual difference for this use case. There might however be other uses of ControlList where it matters. As an implementation detail of this "workaround" of using double, it could perhaps also be nice to implement timepos_t operator* (or operator*=) for double. But I'm not sure we want floating point support in timepos_t. An alternative (and better) solution would be to convince the fraction multiplication to use 128 bits. It is essential to avoid overflow - mainly in static analysis, alternatively as runtime checks or asserts. --- libs/evoral/ControlList.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/evoral/ControlList.cc b/libs/evoral/ControlList.cc index c8246ecc51..90028824bc 100644 --- a/libs/evoral/ControlList.cc +++ b/libs/evoral/ControlList.cc @@ -359,8 +359,9 @@ ControlList::list_merge (ControlList const& other, boost::functionwhen = (*i)->when.operator* (factor); + (*i)->when = timepos_t::from_superclock ((*i)->when.val() * double_factor + 0.5); } mark_dirty ();