ardour
ardour copied to clipboard
Ardour
Fixed overflow in std::numeric_limits<Temporal::Beats>::max()
This fixes a bug where dragging a MIDI note with the mouse would cause it to disappear from view.
This is a fix for a real problem, but it's not a correct fix. Any value should be acceptable as either ticks or beats, and ::normalize() should handle it.
Okay. I guess that means ::normalize() should allow the number of ticks to be greater than PPQN if the number of beats is at its maximum possible value. I will make that change and then check to make sure there isn't any code that depends on the number of ticks always being less than PPQN.
Alternatively, I could make the beats an int64_t internally, while still only accepting int32_t as constructor parameters. That way the ticks could always be less than PPQN. That seems like a bit of a kludge, though, so I will go with the former approach unless told otherwise.
It's not acceptable to expand either field to 64 bits. The entire value needs to fit within 64 bits.
On Tue, Oct 17, 2017 at 3:25 PM, allen-marshall [email protected] wrote:
Okay. I guess that means ::normalize() should allow the number of ticks to be greater than PPQN if the number of beats is at its maximum possible value. I will make that change and then check to make sure there isn't any code that depends on the number of ticks always being less than PPQN.
Alternatively, I could make the beats an int64_t internally, while still only accepting int32_t as constructor parameters. That way the ticks could always be less than PPQN. That seems like a bit of a kludge, though, so I will go with the former approach unless told otherwise.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Ardour/ardour/pull/384#issuecomment-337342771, or mute the thread https://github.com/notifications/unsubscribe-auth/ADrXg8NpeTsXuSmEMfC62D6iuJXmnUT-ks5stP8kgaJpZM4P8kJo .
I have updated my fix so that Beats normalization avoids overflow when possible. The updated code tries to keep _ticks in the range [0, PPQN) (similar to ngeiswei's approach in PR #381 ), except when doing so would cause _beats to overflow. It should now be possible to use any int32_t for either beats or ticks without overflowing. I used int64_t to handle some intermediate calculations, but _beats and _ticks are still stored as int32_t. I also added a unit test for Temporal::Beats. Please let me know if further changes are needed.
Apologies for leaving this hanging around for so many years. In the end, I changed Beats to use a single 64 bit tick counter, and thus addressed this issue in an entirely different way. Thanks for the ideas and input.