allow escape loops seeking backwards

[daschuer]

This fixes https://bugs.launchpad.net/mixxx/+bug/1716897
Now you can jump back to cue, or any other position and the loop remains active.

This discovers also a "bug", that is jumps back to loop after changing the direction.
In this case we are suddenly "after" the loop.
How should we handle this? Normaly we disable the loop if we jump after it. In this case it seems also be wrong.

[daschuer]

https://bugs.launchpad.net/mixxx/+bug/1669500 describes also the direction change issue.

[ronso0]

Cool, thanks for taking care of this.
Will test later on

I have found a rounding issue when playing at fast rates.
OTOH this does not explain why it is fixes with FFmpeg 3.1

Do you mean 'fast rates' when scratching or just playing fast?

[daschuer]

Any fast speed. In you case scratching.

[ronso0]

Yeah, both issues are fixed!
I just tested with the files backward-scratching failed with (also failed with FFmpeg3.1).
Will test this for some more days.

[ronso0]

I doubt the loop should stay active when escaping it, either back or forth.
Loop-catch functionality should IMO be activated explicitly because it essentially (dramatically?) changes the playback flow. But here it's not activated on purpose, it's just 'still On' although the user escaped it.

So "bug or feature?" depends on the use use case. This is mine:

• bring a new song in
• fade out old song with a loop (not necessarily at the end)
• let the new song play (loop stays active, mostly because I forget about it)
• bring in the old song again from a Cue/Hotcue
• at that point I don't want to play the same loop again to repeat the former transition, so depending on where the loop was placed, it might screw up the replay of the old song and the new transition (if I forget the loop again...)

[daschuer]

We have introduced this catching loop behaviour here #1187
I think we did not consider your use case. Thinking about it again it is probably more clear to disable the loop always. The user can in any case Enable/Disable the loops as he wants, without jumping into the loop if the playposition is before the loop.

[daschuer]

Ready! Now it works nice.
The Loop is now always disabled if you jump out of it. A jump from outside to outside position does not change the enable state of the loop.
This fixes also a bug that it could not enable a stored loop of a newly loaded Track.

[ronso0]

When I just tested again I noticed clicking "reloop" has no effect anymore when the play position is behind the loop. Before, it would jump from the later play pos to the loop_in position and enable the previously set loop.

What is the intention of "Allow scratching in front of an enabled loop"?
My expectation was that if scratching in front of an enabled loop, wouldn't get you caught in the loop as long as scratching is True.

[daschuer]

Oh, I will have a look.

"Allow scratching in front of an enabled loop" was a commit, fixing a regression from a previous commit.

[daschuer]

Reloop works again.

[ronso0]

Yes, both bugs are fixed and reloop works again.
But.....while playing around with it I discovered another bug (no regression, it's also in master):

• play a track, set a loop somewhere in the middle of the track
• disable loop, play from the track from the beginning
• re-enable previous loop way ahead of play position
• press loop_halve on controller or in GUI
• play position jumps to loop_in

[uklotzde]

I agree. The loop might be short and setting the loop in marker should also enable the loop while entering it. If I press the in marker again within a loop it will be adjusted and the remaining part should be looped. Of course, this only applies if the loop in marker is strictly before an already set loop out marker.

The other question: Should pressing the loop out marker beyond an already activated loop again really jump to the beginning of the loop like pressing reloop? Or wouldn't it be more reasonable to extend the loop up to this point alike pressing the loop in button which will shorten the loop after this point. The loop stays activated, but since playing forward we will not entering it.

And then all this must work mirrored when playing in reverse. I did not test this.

[daschuer]

The other question: Should pressing the loop out marker beyond an already activated loop again really jump to the beginning of the loop like pressing reloop? Or wouldn't it be more reasonable to extend the loop up to this point alike pressing the loop in button which will shorten the loop after this point.

I do not understand the question. You cannot set a loop out marker behind an activated loop. You can also not set a loop out marker before a loop. You can only set a loop out marker behind a loop in marker. Since we activate the loop, it is quite natural that the playposition jumps to loop in as usual.

I have tried to hard make everything working in forward and backward direction, but it was not possible in every detail, because of the different loop jump out and loop catching logic. IMHO this part works just right though.

[uklotzde]

I don't use loops very often, so don't take my questions too serious ;)

LGTM. And since everyone seems to be happy we are ready to merge this work, aren't we? I just want to wait until the builds have completed. Just in case.

[daschuer]

Thank you :-)

[uklotzde]

Tests enter an infinite loop

[daschuer]

ready.

[ronso0]

Shall I test it again with my controller?

[daschuer]

Yes, an other test can't hurt. Thank you.

[uklotzde]

Now tests are failing:

[  FAILED  ] 9 tests, listed below:
[  FAILED  ] LoopingControlTest.LoopOutButton_QuantizeDisabled
[  FAILED  ] LoopingControlTest.LoopOutButton_QuantizeEnabledNoBeats
[  FAILED  ] LoopingControlTest.LoopOutButton_AdjustLoopOutPointOutsideLoop
[  FAILED  ] LoopingControlTest.LoopOutButton_AdjustLoopOutPointInsideLoop
[  FAILED  ] LoopingControlTest.ReloopToggleButton_TogglesLoop
[  FAILED  ] LoopingControlTest.LoopScale_DoublesLoop
[  FAILED  ] LoopingControlTest.LoopDoubleButton_DoesNotResizeManualLoop
[  FAILED  ] LoopingControlTest.LoopHalveButton_DoesNotResizeManualLoop
[  FAILED  ] LoopingControlTest.LoopMoveTest

[daschuer]

OK, now it should be done. At least tests are not failing (sorry for not testing them before).
Now we have double everywhere and I managed to get rid of most these odd +2 -2 statements.

[uklotzde]

Is still see one test failing due to rounding errors (GCC 7.2.1):

src/test/looping_control_test.cpp:530: Failure
Value of: m_pLoopEndPoint->get()
  Actual: 300
Expected: 300
src/test/looping_control_test.cpp:555: Failure
Value of: m_pLoopEndPoint->get()
  Actual: 300
Expected: 300
...
[  FAILED  ] LoopingControlTest.LoopMoveTest

[uklotzde]

Only rounding errors. With EXPECT_NEAR and a delta of 10⁻⁹ the tests pass.

[daschuer]

Interesting that this fails compiler dependent.

[uklotzde]

I'm still a bit concerned about the complete removal of those strange +/-2 adjustments. The meaning of the code has changed and the behaviour should be different than before. On the other hand it seems to work when using floating point calculations without the adjustments.

[uklotzde]

LGTM. Thank you, Daniel.