Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong visual play position when inside loop #11840

Merged
merged 8 commits into from Sep 1, 2023

Conversation

JoergAtGithub
Copy link
Member

@JoergAtGithub JoergAtGithub commented Aug 19, 2023

Closes: #11836

The loop is now considered in VisualPlayPosition::getAtNextVSync which determines the visual play position used fo the waveform rendering. The play position indicator is no always inside an active loop. Forward and reverse playing direction is considered.

It wasn't neccessary to change VisualPlayPosition::getPlaySlipAtNextVSync because this is only used for the spinnies, which do not visualize loops. Therefore no special handling for slip position was necesarry.

Furthermore I unified some odd sympol naming in the VisualPlayPosition code.

src/engine/enginebuffer.cpp Outdated Show resolved Hide resolved
@m0dB
Copy link
Contributor

m0dB commented Aug 19, 2023

Nice to see it was indeed straightforward and could be done with a few changes!

@daschuer
Copy link
Member

It wasn't neccessary to change VisualPlayPosition::getPlaySlipAtNextVSync because this is only ...

The slip position also follows loops. Even though there are no loop markers on the spinners, it would be correct to consider loops as well.

@JoergAtGithub
Copy link
Member Author

The slip position also follows loops

This seems to be another bug. According to our manual, not to follow loops is explicitly the purpose of the Slip mode:

When active, the playback continues muted in the background during a loop, reverse, scratch, etc.

@ronso0
Copy link
Member

ronso0 commented Aug 19, 2023

I think @daschuer refers to the situation where a loop is enabled before slip mode is enabled, see #11435

@ronso0
Copy link
Member

ronso0 commented Aug 19, 2023

FWIW I noticed the offset with quantized full-track loops aka repeat where the playpos would be before track start for a glimpse, and then jump to the actual engine playpos.
Hence I was thinking about using LoopingControl::nextTrigger (which considers both loops and repeat) when I gave my pessimistic view on a quick fix.

@JoergAtGithub
Copy link
Member Author

It wasn't neccessary to change VisualPlayPosition::getPlaySlipAtNextVSync because this is only ...

The slip position also follows loops. Even though there are no loop markers on the spinners, it would be correct to consider loops as well.

In general I agree, but this is out of scope of this waveform PR!
I tried to create a follow-up PR for the spinnies, but I struggled with the behaviour of the Slip Mode in general. It just not does what I expect from a Slip mode, and the documentation doesn't helped me to get a better understanding.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work with catching loops in font of the play position. In this case the waveform shows already the loop.


double interpolatedPlayPos = data.m_playPos + offset * data.m_playRate;

if (data.m_loopEnabled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this must only applied when the data.m_playPos is on one side of the loop marker and the interpolatedPlayPos is on the other side.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've commited a fix.

@daschuer
Copy link
Member

You can use exactly the same code in getPlaySlipAtNextVSync() for the spinnies for both positions slip and normal.
Both will follow the loop.

@JoergAtGithub
Copy link
Member Author

You can use exactly the same code in getPlaySlipAtNextVSync() for the spinnies for both positions slip and normal. Both will follow the loop.

I don't think it's that easy for the slip position, because it depends on the order of activation of loop and slip mode. I worked already on a follow-up PR for the spinnies, but than found: #11844

@JoergAtGithub
Copy link
Member Author

Based on #11848 I could easily add the code for the slip indicator in the spinnies too.
The new code also fixes the visual bug, that the slip indicator appeared for a moment at 12 o'clock position when enabling it.

@JoergAtGithub JoergAtGithub force-pushed the playindicator branch 2 times, most recently from 985ba70 to dfbb19e Compare August 21, 2023 20:37
@JoergAtGithub
Copy link
Member Author

Ping

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Two remaining comments.

@@ -58,6 +58,12 @@ class LoopingControl : public EngineControl {
void setLoop(mixxx::audio::FramePos startPosition,
mixxx::audio::FramePos endPosition,
bool enabled);
mixxx::audio::FramePos getLoopStartPosition() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loop Start and End should be only read atomically. This avoids that the position can slip out during moving the loop.
In this case LoopInfo should be returned, or both values in a single call.


enum class SlipModeState {
Disabled = 0,
Armed = 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a brief comment about armed?

@JoergAtGithub
Copy link
Member Author

Requested changes are done!

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants