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

passthrough: stop rendering waveforms, disable Cue/Play indicators #4793

Merged
merged 2 commits into from
Jun 13, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jun 11, 2022

When loading a track while passthrough is active the waveforms are cleared.

  • For consistency, let's clear them also when passthrough is activated while a track is loaded.
    (makes lp:1567645 Waveform gain changes if passthrough is enabled obsolete)
  • Stop CUE and Play indicators when passthrough is activated, update when disabled. Removes distraction and makes moving CUE less likely IMO.

This is only the minimal fix without with minimal UI changes. The complete solution is in #4794

  • some related ToDos to avoid confusing behaviour

@ronso0 ronso0 added the ui label Jun 11, 2022
@ronso0 ronso0 added this to the 2.3.3 milestone Jun 11, 2022
@ronso0 ronso0 mentioned this pull request Jun 11, 2022
5 tasks
@ronso0 ronso0 marked this pull request as draft June 11, 2022 21:47
@ronso0 ronso0 force-pushed the waveform-passthrough-enabled branch from 7c618c6 to af8f13c Compare June 12, 2022 12:24
@ronso0 ronso0 marked this pull request as ready for review June 12, 2022 12:25
@ronso0 ronso0 added the engine label Jun 12, 2022
@ronso0 ronso0 changed the title passthrough: when enabled stop rendering waveforms passthrough: stop rendering waveforms, disable Cue/Play indicators Jun 12, 2022
m_visualPlayPos->setInvalid();
if (!m_passthroughWasEnabled) {
// Disable CUE and Play indicators
m_pCueControl->resetIndicators();
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this block to slotPassthroughChanged() and get rid of m_passthroughWasEnabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️
Yes 👍

@ronso0 ronso0 force-pushed the waveform-passthrough-enabled branch from af8f13c to f58a2ed Compare June 12, 2022 21:42
m_pPassthroughEnabledControl->connectValueChanged(this,
&EngineBuffer::slotPassthroughChanged,
Qt::DirectConnection);
m_passthroughEnabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why false instead of m_pPassthroughEnabledControl->toBool()

Copy link
Member Author

Choose a reason for hiding this comment

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

The EngineDeck constructor creates the "passthrough" ControlPushButton and instantiates EngineBuffer, so "passthrough" is still false here.
Anyway, you're right, someone may change it so "passthrough" is initiated true then this wouldn't match.

@@ -343,7 +343,8 @@ class EngineBuffer : public EngineObject {
// This ControlProxys is created as parent to this and deleted by
// the Qt object tree. This helps that they are deleted by the creating
// thread, which is required to avoid segfaults.
ControlProxy* m_pPassthroughEnabled;
ControlProxy* m_pPassthroughEnabledControl;
bool m_passthroughEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why store and manage a redundant state instead of simply accessing the current state through m_pPassthroughEnabledControl->toBool()?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC reading a bool member is faster than querying a control on every call?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this would be necessary then ControlProxy should instead store a local copy of the value, but not all using code again and again.

I never understood how all the ControlThings are intended to be used safely and correctly.

Copy link
Member

Choose a reason for hiding this comment

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

There is almost no overhead on a 64 bit system, except the function call. On a 32 bit system a ring buffer is used, which causes one indirection when reading and some ring maimtannance when writing.

Copy link
Member Author

Choose a reason for hiding this comment

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

No overhead, so I drop the bool.

@ronso0
Copy link
Member Author

ronso0 commented Jun 12, 2022

Thank you both for your review.
I'll collect the small proposed changes, commit --amend and force-push, so please let me know when you consider it ready for merge (incl. the changes).

@ronso0 ronso0 force-pushed the waveform-passthrough-enabled branch from f58a2ed to 1b0fb5e Compare June 13, 2022 10:06
@ronso0
Copy link
Member Author

ronso0 commented Jun 13, 2022

fixed, rebased.
Ready for final review.

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.

@daschuer daschuer merged commit 2441b17 into mixxxdj:2.3 Jun 13, 2022
@ronso0 ronso0 deleted the waveform-passthrough-enabled branch June 13, 2022 14:18
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.

3 participants