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

Sync Lock: Don't seek phase when disabling sync #4169

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Aug 1, 2021

No description provided.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1087668649

  • 3 of 3 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 26.018%

Totals Coverage Status
Change from base Build 1087078133: 0.007%
Covered Lines: 20009
Relevant Lines: 76904

💛 - Coveralls

@ronso0 ronso0 added engine and removed code quality labels Aug 1, 2021
@ronso0 ronso0 added this to the 2.4.0 milestone Aug 1, 2021
@Holzhaus
Copy link
Member

Holzhaus commented Aug 1, 2021

Ist the main target branch correct? Or does this affect 2.3?

@@ -389,6 +393,7 @@ class EngineBuffer : public EngineObject {
QAtomicInt m_iEnableSyncQueued;
QAtomicInt m_iSyncModeQueued;
ControlValueAtomic<QueuedSeek> m_queuedSeek;
bool m_previousBufferSeek = false;
Copy link
Member

Choose a reason for hiding this comment

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

If this is accessed from multiple threads it should be an atomic for proper memory barriers.

Copy link
Member Author

@ywwg ywwg Aug 4, 2021

Choose a reason for hiding this comment

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

It isn't, this value is only written in the processSeek function and used by tests in a non-threaded context

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.

The fix itself works and locks good. Thank you.
The issue in 2.3 is slightly different.

@ywwg
Copy link
Member Author

ywwg commented Aug 6, 2021

anything more needed here?

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.

No, Thank you. LGTM

@daschuer daschuer merged commit 66473bf into mixxxdj:main Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants