Add FramePos type and use it for beatgrid creation #4043

Holzhaus · 2021-07-01T14:49:29Z

Ripped out of #2961.

We already store frame positions in the protobuf object, so we can also use the new framepos type for that.
I only added the new type to the analyzer, beatutils and the beatgrid creation method to keep the size bearable.

src/track/beatfactory.cpp

uklotzde

The usage of std::isnan is probably invalid and should be tested.

src/track/beatmap.cpp

uklotzde · 2021-07-01T19:17:02Z

src/track/beatmap.cpp

-    startBeat.set_frame_position(
-            static_cast<google::protobuf::int32>(samplesToFrames(startSample)));
-    stopBeat.set_frame_position(static_cast<google::protobuf::int32>(samplesToFrames(stopSample)));
+    Beat startBeat = beatFromFramePos(mixxx::audio::FramePos::fromEngineSamplePos(startSample));


beatFromFramePos(mixxx::audio::FramePos::fromEngineSamplePos(value)) is invoked multiple times. Extract as a small, inlined utility function?

I noticed that there is one special case where .toFullFrames() is invoked before passing the value into beatFromFramePos(). It might make sense to extract this as a separate function with a dedicated name to isolate the occurrences and have all possible variations close to each other.

beatFromFramePos always strips away fractional frame positions. In fact, we can always invoke .toFullFrames(). But I wasn't sure if that is intended.

The location where "toFullFrames" is called explicitly is the location where we previously called "std::floor" explicitly.
I wasn't sure if that is just coincidence or if it's indeed desired. But if I uncomment the debug assertion that checks that the frame position is not fractional, some tests fail. The qeustion is: Do we always want to discard frame positions silently, or should be make it explicit and re-add the assertion?

But that kind of investigation is out of scope for this PR IMHO.

The name of the function is also not very intuitive. But let's just leave it as is for the moment.

If there are known issues that must be resolved before a release we might also use FIXME as a marker to distinguish mandatory from optional TODOs, e.g. to either enable the debug assertion or update the documentation. This could be handy to quickly find these spots when we decide to merge unfinished work.

src/audio/frame.h

src/track/beatmap.cpp

uklotzde

@daschuer might have a final look.

Be-ing · 2021-07-01T22:27:00Z

Thank you for taking up this tedious cleanup.

daschuer

Thank you very much for this PR. It is a good way forward. I have left some comments.

src/analyzer/plugins/analyzerqueenmarybeats.cpp

daschuer · 2021-07-02T06:34:44Z

src/audio/frame.h

+        return std::modf(value(), &integerPart) != 0;
+    }
+
+    [[nodiscard]] FramePos toFullFrames() const {


From the function mame it is not clear if it is foor or ceil or round.
Let's either remove this function or rename it.
Do we need the other candidates as well?

Floor is not int cast.

When I mentioned the naming is not ideal I had something like toLowerFrameBoundary() in mind.

@uklotzde Done.

I guess that for the future we need a more sophisticated solution:

If the value is close (< epsilon) to a beat then use std::round

Otherwise use either the lower or upper boundary, depending on the use case.

The value of epsilon may also vary.

src/audio/frame.h

src/test/frametest.cpp

daschuer · 2021-07-02T07:00:19Z

src/test/frametest.cpp

+    EXPECT_FALSE(mixxx::audio::FramePos(99999).isFractional());
+    EXPECT_FALSE(mixxx::audio::FramePos(-99999).isFractional());
+    EXPECT_TRUE(mixxx::audio::FramePos(128.5).isFractional());
+    EXPECT_TRUE(mixxx::audio::FramePos(135.67).isFractional());


Please add her also a check with Nan values and denormals.

What should it return? Is nan fractional?

Let's accept that the result is undefined. It doesn't matter and passing NaN is a bug. Just add a debug assertion to detect this case.

src/track/beatfactory.cpp

daschuer · 2021-07-02T07:36:27Z

src/track/beatmap.cpp

-    beat.set_frame_position(static_cast<google::protobuf::int32>(samplesToFrames(dSamples)));
+    Beat beat = beatFromFramePos(
+            mixxx::audio::FramePos::fromEngineSamplePos(dSamples)
+                    .toFullFrames());


This changes the code for negative values because of internal floor()

If negative values are not allowed we should check for that. Otherwise rounding to frame boundaries should always be done in the same direction. Depending on the use case but independent of the sign. Rounding in different directions is probably wrong.

I was wondering if std::trunc is preferable because it always rounds towards zero. But no idea.

On second thought: trunc may lead to tiny Bpm changes around zero because the distance between beats is smaller. So using floor is the right call IMHO.

Strictly speaking we have now a new beat map version.
But since we have now beats below zero it does not matter.

Holzhaus · 2021-07-02T13:29:34Z

Anything else to do here? I'm already working on a PR that uses frame positions for the beats API.

daschuer · 2021-07-02T16:51:51Z

Thank you very much for jumping in here.

hacksdump and others added 15 commits July 1, 2021 12:32

Frame: Add frame class to avoid sample/frame ambiguity

18382f5

Frame: Add artithmetic and logical operators

6df3614

Frame: Add multiply and divide operations

34d1f9d

Frame: Replace forward declaration with include

92161c5

Frame: Refactor and rename to FramePos and FrameDiff_t

4a7bdc5

FramePos: Use constant for invalid frame instead of -1

e3124ff

FramePos: Use C++ style limits

e98439f

FramePos: Use constant for starting frame pos

eb550fa

FramePos: Use NaN instead of negative infinity

b048478

FramePos: Add Qt declarations

9ae96f2

FramePos: Define static start and invalid constants

3d9df08

FramePos: Move class alongside other audio helper classes

7a05af6

FramePos: Clean up comments

db6b5f6

FramePos: Remove d prefix for doubles

aa17102

FramePos: Make FramePos invalid when no value is given

f79e907

Holzhaus added engine code quality labels Jul 1, 2021

github-actions bot added the library label Jul 1, 2021

Holzhaus commented Jul 1, 2021

View reviewed changes

src/track/beatfactory.cpp Show resolved Hide resolved

Holzhaus force-pushed the frames branch from 60b44f1 to c70d208 Compare July 1, 2021 15:02

Holzhaus added 6 commits July 1, 2021 17:19

FramePos: Add isValid method to check if the position is valid

4db7cb2

FramePos: Add fromEngineSamplePos helper method

b82ba59

FramePos: Rename getValue() to just value()

e7e39e4

FramePos: Add helper method to truncate to full sample positions

0191a50

Beats: Use frame positions for beatmap/beatgrid construction

af40d4f

BeatMap: Reduce back-and-forth conversions between samples/frames

cd8b88c

Holzhaus force-pushed the frames branch from c70d208 to cd8b88c Compare July 1, 2021 15:23

uklotzde suggested changes Jul 1, 2021

View reviewed changes

Holzhaus added 2 commits July 1, 2021 22:04

BeatMap: Add inline to internal utility functions

69c3717

Frame: Add constexpr to some methods

f808edb

Holzhaus added 3 commits July 1, 2021 22:24

FramePos: Fix wrong fractional check

fb2be0b

FramePos: Use util_isnan to fix broken NaN detection

e4f9b22

FramePos: Add tests

9d2811c

github-actions bot added the build label Jul 1, 2021

Holzhaus requested a review from uklotzde July 1, 2021 20:35

Holzhaus mentioned this pull request Jul 1, 2021

Use mixxx::Bpm for some Beats functions #4044

Merged

uklotzde reviewed Jul 1, 2021

View reviewed changes

src/track/beatmap.cpp Outdated Show resolved Hide resolved

BeatMap: Use in const reference in for loop to avoid copy

35a5ccd

uklotzde reviewed Jul 1, 2021

View reviewed changes

src/track/beatmap.cpp Show resolved Hide resolved

uklotzde approved these changes Jul 1, 2021

View reviewed changes

daschuer requested changes Jul 2, 2021

View reviewed changes

daschuer reviewed Jul 2, 2021

View reviewed changes

Holzhaus added 5 commits July 2, 2021 11:45

AnalyzerQueenMaryBeats: Rename m_stepSize to m_stepSizeFrames

e671a6c

FrameTest: Check denormals and NaN

ad42583

FramePos: Make infinite frame positions invalid

c26b018

BeatMap: Add helper function and add assertion for fraction positions

70416aa

FramePos: Assert validity in isFractional()

a555ae4

Holzhaus requested a review from daschuer July 2, 2021 11:31

daschuer approved these changes Jul 2, 2021

View reviewed changes

daschuer merged commit 5dc50d1 into mixxxdj:main Jul 2, 2021

Holzhaus mentioned this pull request Jul 3, 2021

FramePos improvements #4053

Merged

Holzhaus added this to In Progress in Semanting Type Refactoring via automation Jul 6, 2021

Holzhaus moved this from In Progress to Done in Semanting Type Refactoring Jul 6, 2021

Holzhaus added this to Done in Beats Refactoring Jul 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FramePos type and use it for beatgrid creation #4043

Add FramePos type and use it for beatgrid creation #4043

Holzhaus commented Jul 1, 2021

uklotzde left a comment

uklotzde Jul 1, 2021

Holzhaus Jul 1, 2021

uklotzde Jul 1, 2021

uklotzde left a comment

Be-ing commented Jul 1, 2021

daschuer left a comment

daschuer Jul 2, 2021

daschuer Jul 2, 2021

uklotzde Jul 2, 2021

Holzhaus Jul 2, 2021

uklotzde Jul 2, 2021

daschuer Jul 2, 2021

Holzhaus Jul 2, 2021

uklotzde Jul 2, 2021

Holzhaus Jul 2, 2021

daschuer Jul 2, 2021

uklotzde Jul 2, 2021

Holzhaus Jul 2, 2021

Holzhaus Jul 2, 2021

daschuer Jul 2, 2021

daschuer Jul 2, 2021

Holzhaus commented Jul 2, 2021

daschuer commented Jul 2, 2021

Add FramePos type and use it for beatgrid creation #4043

Add FramePos type and use it for beatgrid creation #4043

Conversation

Holzhaus commented Jul 1, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde left a comment

Choose a reason for hiding this comment

Be-ing commented Jul 1, 2021

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Jul 2, 2021

daschuer commented Jul 2, 2021