Bpm: Add a beatLength() method that returns a duration #4138
Conversation
|
Duration is a qint64 in nanoseconds It is not compatible to our play position in double and we are not able to lossless convert one to another. Even though the difference is probably marginal, it becomes notable if we iterate over a track. So I vote for using a double duration type for with a maximum precision. Can we refactor the duration time to double? Unfortunately a qint64 dos not fit to a double, but I think we can't find a case where this is relevant in Mixxx. |
| @@ -62,6 +63,11 @@ class DurationBase { | |||
| return static_cast<double>(m_durationNanos); | |||
| } | |||
|
|
|||
| // Returns the duration as an integer number of nanoseconds. | |||
| constexpr double toFrames(mixxx::audio::SampleRate sampleRate) const { | |||
There was a problem hiding this comment.
The duration should not know anything about frames and sample rates.
There was a problem hiding this comment.
Writing duration.toDoubleSeconds() * sampleRate is tedious and not that expressive, so my initial idea was to "express a duration in frames". But I agree that a frame count is not a during without knowing the sample rate.
The alternative would be to add a method to the SampleRate class instead:
constexpr double SampleRate::frameCount(mixxx::Duration duration) {
return duration.toDoubleSeconds() * value();
}And using it would look like this:
constexpr auto bpm = mixxx::Bpm(120);
constexpr auto sampleRate = mixxx::audio::SampleRate(44100);
const double beatLengthFrames = sampleRate.frameCount(bpm.beatLength());What do you think?
There was a problem hiding this comment.
Conceptual Dependency Graph: Duration <- Sample Rate <- Frame
Sample rate knows about duration because of frequencies. But it doesn't need to know anything about frames.
Frames know about sampling because their purpose is to represent a continuous audio signal as discrete, sampled values.
Therefore any calculation that involves frames should not taint neither duration nor sample rate code.
There was a problem hiding this comment.
Why not double operator*(Duration, SampleRate) (and its twin with switched arguments) in samplerate.h? The unit of the resulting value depends on the context.
I would not recommend to do this. Then you will get rounding errors for integer durations measured in milliseconds or microseconds that currently can be represented exactly. Better introduce a new type for this purpose to distinguish between system and audio duration. |
Not if we normalize the double value to ns. |
Ok, if we store nanoseconds internally then using a single representation would work. |
|
A double can store integers exact up to 2^53 1,000,000,000,000,000 If we pick ns as base we are exact convertable for all times from |
Instead of calculating it manually, i.e. `60.0 / bpm`, a properly-named dedicated method will make it apparent what we are calculating to anyone who skims over the code.
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
I think semantically this makes sense and would look nicer in code. Right now, the
Bpm::beatLength()method returns amixxx::Duration. I also added a methodDuration::toFrames(SampleRate)to make the code even easier to read.The
mixxx::Durationclass uses a 64 bit integer for storing full nanoseconds. Does this precision suffice? I don't know. Some tests fail due to this loss of precision, because they hardcode a lot of decimal points. Please let me know if we should we change the tests, not usemixxx::Duration, or just not make this change at all?Related Zulip Discussion: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Semantic.20type.20refactoring/near/246427762