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.

Sign up for GitHub

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

Jump to bottom

ReadAheadManager: fix loop wraparound reader condition #11717

Merged
merged 5 commits into from
Jul 9, 2023
Merged

ReadAheadManager: fix loop wraparound reader condition #11717

merged 5 commits into from
Jul 9, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jul 7, 2023 •
edited
Loading

should fix a regression caused by #11704 / #11532
noticed because HotcueControlTest.SavedLoopMove failed in Coverage check.

Though requested samples shouldn't be negative in the first place, so maybe we overlooked something.

@ronso0
Copy link
Member Author

ronso0 commented Jul 7, 2023

@daschuer please take a look.

@ronso0 ronso0 marked this pull request as draft July 7, 2023 01:37
@daschuer
Copy link
Member

daschuer commented Jul 7, 2023

Thank you. The changes are looking reasonable.
Is kLogger.critical() crashing the release version? Maybe we should make this a warning. And keep the party going.

ronso0
ronso0 commented Jul 7, 2023
View reviewed changes
src/engine/readaheadmanager.cpp Outdated
@@ -160,7 +160,7 @@ SINT ReadAheadManager::getNextSamples(double dRate, CSAMPLE* pOutput,
}
}

if (crossFadeSamples) {
if (crossFadeSamples >= 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

still trying to figure when crossFadeSamples < 0 occurs.
if I get it right this can happen only if the overshoot (fractional frames?) corrects the curr. play pos AND we're looping over the track start, so that loop_read_position > crossFadeStart?
so we actually thow away the overshoot, no?

Copy link
Member

Choose a reason for hiding this comment

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

The code below should do nothing in case of 0, so we can safe the time-

Suggested change
if (crossFadeSamples >= 0) {
if (crossFadeSamples > 0) {

@ronso0
Copy link
Member Author

ronso0 commented Jul 7, 2023

Is kLogger.critical() crashing the release version? Maybe we should make this a warning. And keep the party going.

qFatal() does IIUC, and qCritical() does, too "if the environment variable QT_FATAL_CRITICALS is not empty"
https://doc.qt.io/qt-6/qtlogging.html#qCritical
Don't know why this would be enabled. Do you? Sure we can make it a warning.

@ronso0 ronso0 marked this pull request as ready for review July 7, 2023 10:37
@daschuer
Copy link
Member

daschuer commented Jul 7, 2023

qFatal() does IIUC, and qCritical() does, too "if the environment variable QT_FATAL_CRITICALS is not empty"

Oh I was confused because our assertions are Critical and elevated Fatal for a crash.
So all fine.

daschuer
daschuer requested changes Jul 7, 2023
View reviewed changes
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.

I want still investigate why the value is negative. But that can be done after merge.

src/engine/readaheadmanager.cpp Outdated
@@ -160,7 +160,7 @@ SINT ReadAheadManager::getNextSamples(double dRate, CSAMPLE* pOutput,
}
}

if (crossFadeSamples) {
if (crossFadeSamples >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

The code below should do nothing in case of 0, so we can safe the time-

Suggested change
if (crossFadeSamples >= 0) {
if (crossFadeSamples > 0) {

@ronso0
Copy link
Member Author

ronso0 commented Jul 9, 2023

so this one's ready

daschuer
daschuer approved these changes Jul 9, 2023
View reviewed changes
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.

It turns out that the value has to be negative, because the loop cue is shifted into the pre-roll.
So this is OK.

Unfortunately this issue is also a 2.3 issue. I will add only the fix to the 2.3 branch without the refactoring.

LGTM

@daschuer daschuer merged commit 5c299f5 into mixxxdj:2.4 Jul 9, 2023
daschuer added a commit that referenced this pull request Jul 9, 2023
@daschuer
Don't try to crossfade a negative length
3ec60a7 
This a the backport of the fix from #11717
@ronso0 ronso0 deleted the cachingreader-debug branch July 9, 2023 20:30
@daschuer daschuer added this to the 2.4.0 milestone Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschuer daschuer daschuer approved these changes

Assignees
No one assigned
Labels
regression
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ronso0 @daschuer