Beats: Round positions to lower frame boundary on translate

[Holzhaus]

This fixes the following debug assertion:

DEBUG ASSERT: "!m_endMarkerPosition.isFractional()" in function mixxx::Beats::Beats(std::vector<mixxx::BeatMarker>, mixxx::audio::FramePos, mixxx::Bpm, mixxx::audio::SampleRate, const QString&) at /home/jan/Projects/mixxx/src/track/beats.h:177

[daschuer]

Why we need to round at all?

I think our legacy beat grid has fractional frames anyway and our new format will support them as well. The only issue is when we save them as a beat map because it uses an int32, but that can be done at the final stage just before storing. That way we can easily get rid of it once we have a floating point enabled format.

[uklotzde]

Why we need to round at all?

I think our legacy beat grid has fractional frames anyway and our new format will support them as well. The only issue is when we save them as a beat map because it uses an int32, but that can be done at the final stage just before storing. That way we can easily get rid of it once we have a floating point enabled format.

Please avoid to extend the scope of PRs. This is not the place for discussing a major refactoring.

[daschuer]

First I like to understand the reasons for the rounding. This is required to understand the whole change and decide if this PR is a good solution.

Without this, I would argue to just remove the assertion. This would be also a limited scope.

[Holzhaus]

I think our legacy beat grid has fractional frames anyway and our new format will support them as well. The only issue is when we save them as a beat map because it uses an int32, but that can be done at the final stage just before storing. That way we can easily get rid of it once we have a floating point enabled format.

No, this is not correct. Both legacy formats (beatgrid and beatmap) store frame positions as int32:

message Beat {
  optional int32 frame_position = 1;
  optional bool enabled = 2 [ default = true ];
  optional Source source = 3 [ default = ANALYZER ];
}

message BeatMap {
  repeated Beat beat = 1;
}

message BeatGrid {
  optional Bpm bpm = 1;
  optional Beat first_beat = 2;
}

First I like to understand the reasons for the rounding. This is required to understand the whole change and decide if this PR is a good solution.

Without this, I would argue to just remove the assertion. This would be also a limited scope.

It's a minor issue, but for the non-technical user it's not obvious that unloading + reloading a track can result in a shifted beatgrid due to rounding. Hence, we always rounded the positions when mutating the beatgrid, not when only when saving it.

[daschuer]

No, this is not correct. Both legacy formats (beatgrid and beatmap) store frame positions as int32:

I was referring the fact that you can have a beat at a fractional position when using beat grids, because the number of frames is unlikely a whole denominator of the number of beats.

In case of BeatMap every beat is forced to a int position because of the storage format.

So if we like to convert losslessly from one to another, we need to allow fractional end maker.

It's a minor issue, ...

Ah OK I understand.

But be aware that we have also users, insisting having int BPMs. We can only round one thing, the BPM or the frame.

Currently we have complex code implemented to round the BPM. The rounding to the lower frame here breaks it.
But it will be broken anyway after reloading. So your reasoning for now is OK.

I am only concerned that we build it deep into our business logic and have hard time to remove it once we are on the new format.
Can we build something around it that it is easy to remove later?

But I actually still prefer to just accept the rounding by storing during the transitional period, instead of creating to much work for it now and just proceed with removing the assertion.

[Holzhaus]

I am only concerned that we build it deep into our business logic and have hard time to remove it once we are on the new format.

We had this explicit rounding on mutation with the legacy code, and we have it with the new code. This is just one instance where I forgot to add it. I don't think it makes sense to remove it only here, and I dont want to turn this tiny regression fix into a bigger rework of how frame positions are handled.

But be aware that we have also users, insisting having int BPMs. We can only round one thing, the BPM or the frame.

FWIW I don't think it makes sense to bother our users with a BPM display resolution of more than 2 digits behind the decimal point.

And the inaccuracy due to rounding fractional frame positions would be way less than 0.005 BPM, which means that users won't notice anyway.

But I actually still prefer to just accept the rounding by storing during the transitional period, instead of creating to much work for it now and just proceed with removing the assertion.

I'm not sure how the storage format will look like. Maybe we want to store marker positions as int32 frames as before. Or double frame positions? Or use a time-based format and use int64 nanoseconds?

At this point it would be more work to remove the assertion and be less correct with the legacy format IMHO. And we don't know if it becomes obsolete with the new format. So I suggest to just keep it for now and merge this PR as-is.

[daschuer]

Ok, than let's merge it. LGTM