Cue positions at ~-1.0 are "invalid"

[ywwg]

Bug Description

I noticed that in two of my tracks, the main cue point was not working. After some debugging, I discovered this is because the cue position rounds to -1.0, and in frame.h, -1.0 is the "kLegacyInvalidEnginePosition". Therefore the cue is considered invalid and ignored. It's not even possible to set a new cue position without using Reset / Cue Point. With some tracks, because of beat grid adjustments it is plausible that the main cue point is actually in the silence before the start. (I have some where it is an entire beat or two before the start.) In this case, it is very very close to 1 frame before the start, at position -1.049180327871.

Do we still need this invalid position value check? The comments mention backwards compatibility but it's not clear what situations this is needed. Could NaN or -INF be used instead?

In the meantime I can tweak the beatgrid of these tracks to avoid the problem.

Version

main branch

OS

Ubuntu 22.04

[ywwg]

I see that -1.0 is also kNoPosition. One workaround would be if the engine (after quantizing etc) tries to set a cue point of -1, set it to -2 instead to just avoid the problem.

[ywwg]

mmm unfortunately there are just too many places in code where we ask if frame positions are -1.0, and if they are, we think they are invalid. I'm not sure how to fix this other than to change the invalid value to -inf or similar.

[ywwg]

This change seems to have introduced the idea of -1 being invalid: #4069

[Holzhaus]

This change seems to have introduced the idea of -1 being invalid: #4069

No, invalid sample position was also -1 before that PR, just hard-coded without a constant.

[ywwg]

ok, so would it be possible to change that constant? I have a draft PR here that appears to work, but I don't know all the possible edge cases. I did grep through the code for all instances of -1.0 :) #10994

[JosepMaJAZ]

Wasn't 1.0 and -1.0 supposed to be relative to full track length? I guess the addition of beat-based positioning might make this -1.0 assumption erroneous and a bigger number needs to be found.

[ywwg]

in this case these are frame positions, so the assumption is that you'd never have a cue point (or waveform in point) < 0. But the reality of beat grid detection is not perfect and it's possible that the first beat is detected as being slightly before the 0 point of the track. and as I said above, I have tracks where the initial cue point is a couple of beats before the beginning of the track. So relying on the concept of "values outside the audio waveform are invalid" is not correct.

[JosepMaJAZ]

I agree that "values outside the audio waveform are valid", but it's not only detection.
One can have CD rips of inexact position, or cases of songs where the first bar starts differently than the next one. So a cue on the negative range is a wanted feature.
Mixxx allow rewinding before the start. Setting a cue there is just an expected feature.
Using frame positions or samples is not the question. Obviously the decoder needs to be aware that not all positions are decoded audio.

Back to the -1.0 and -inf, what will a controller script get?
The description of cue_clear mentions -1

[Holzhaus]

We have 2 types of positions:

1. Frame positions
2. Sample positions

Before the FramePos refactor, most places in the engine used checks like if (samplePos < 0) return; or if (samplePos == -1.0) return;. The refactoring migrated to frame positions, and these checks were replaced with checks like if (!framePos.isValid()) { return; }.
There is also the kInvalidFramePos constant which uses NaN internally, not -1.0. But this one should only be used to invalidate a position, not for comparisons.

However, -1.0 is still used to denote invalid sample positions. This is legacy code, and unfortunately we cannot change this, because we expose the sample positions as part of the CO interface.

Exposing sample positions instead of frame positions is a bad idea in and of itself. But in addition to that, we have a lot of controller scripts that rely on invalid sample positions equalling -1.0. They may use this to to determine if a track is loaded or unset a cue. Hence, I think we need to live with that.

We could add a bunch of new COs that use frame positions instead of sample positions and deprecate the old sample based ones (but keep them working).

[ywwg]

So I think then we should just have workaround code to detect if the user is trying to set a cue point (or hotcue, or loop point) to -1 and round it to 0 or -2, whichever is closer. The error will be infinitesimal and won't cause any sync issues

[Holzhaus]

So I think then we should just have workaround code to detect if the user is trying to set a cue point (or hotcue, or loop point) to -1 and round it to 0 or -2, whichever is closer. The error will be infinitesimal and won't cause any sync issues

To be honest we should probably round all valid sample positions to even values. There is no valid reason to have a cue point between the interleaved left and right stereo sample... or am I missing something?

[daschuer]

This is at least a chance for a workaround.

-1 sample position is just a -0.5 frames position which can happen because we select cue positions paused on samples after the scaler and store it at positions before the scaler.

A not that realistic, but easy case is 50 % speed, where you have doubled all frames and so you can stop and place a cue at the -1 sample position.

But we can easily round up to 0 or just to -0,9999 in this case.
For quantized cued or loops the float expression is usefull, since the beats are as well float to support snapping to a integer BPM value.

[ywwg]

Unfortunately there are places in code where we round the value, so anything from -1.4999 to -.4999 become -1.0. So it's a slightly larger range of values and making it -.999 is not sufficient.

I've started experimenting with the loop and cue code to insert a helper function. There a lot of places where we have to add the check, so I can't guarantee 100% success, but at least this would fix the case I am seeing right now. It's clearly a rare edge case so I don't think it needs bulletproof fixing

[ywwg]

To be honest we should probably round all valid sample positions to even values. There is no valid reason to have a cue point between the interleaved left and right stereo sample... or am I missing something?

agree -- this applies to a frame value of -1.0, not a sample value.

[ywwg]

I think the bug here is that cues can get saved with fractional frame values. I think if we can avoid that happening then the bug is fixed.

(This is incorrect -- because loops must be beat perfect, they may not be sample-perfect. So fractional-frame cue points are valid)

[ywwg]

ahhhhh I think I have it.