-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Scalers: Replace the unneded flushing code with silence passing #11152
Conversation
The code should be unreachable anyway. Assert for this and remove the wrong early exit by break.
Assert that assumption.
This does not fix a bug, but is a foundation for another pending bug with different offsets of different sound touch versions. |
Done |
I'm sorry, I currently don't have the time to dig into the code enough to confidently reason about the changes. Someone else has to take over. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this is not a code review, I just wanted to know if this PR maybe fixes #11381 and hit some DEBUG_ASSERT while moving the loop and seeking in the track.
I polished that commit and opened daschuer#88 |
buffer scalers: consider getNextSamples() returning 0 after loop move
It took me a while to find time to verify your findings. It looks very good now. Thank you. |
SampleUtil::clear( | ||
m_buffer_back, | ||
getOutputSignal().frames2samples(iLenFramesRequired)); | ||
deinterleaveAndProcess(m_buffer_back, iLenFramesRequired); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't fully understand all scaler methods: do we need a break;
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No break, because we nee to continue the loop until we have received all samples.
Adjusting a comment faa1f6c |
Done |
Code LGTM, no regressions spotted, test pass, a bug fixed. (yet I'm hesitant because this touches a sensitive part of the engine, I'm not 100% sure I overlook all consequneces tbh, also this contains a few commits by me) |
Alright then... Thanks @daschuer! |
This is a leftover form an earlier version of the engine, that is no longer needed. If this assumption is wrong the engine would have used old samples left in the buffer In. We now assert for this condition and clear the buffer if the assumption is wrong.