EngineFilterDelay: clamp wrong delay values #4869
Conversation
| @@ -29,6 +29,13 @@ class EngineFilterDelay : public EngineObjectConstIn { | |||
|
|
|||
| void setDelay(unsigned int delaySamples) { | |||
| m_delaySamples = delaySamples; | |||
|
|
|||
| // When mixxx will support other channel count variants than stereo, | |||
There was a problem hiding this comment.
You may put a static_assert for this condition as well. That will fail the build in that case.
There was a problem hiding this comment.
Can you also assert here that the delay is devide-able by the channel count?
This way we can below just use < SIZE which is easier to understand.
There was a problem hiding this comment.
Agree, that is really more clear way.
| // the kMaxDelay has to be divisible by the number of channels. | ||
| // Otherwise, channels may be swapped. | ||
| VERIFY_OR_DEBUG_ASSERT(m_delaySamples <= SIZE - mixxx::kEngineChannelCount) { | ||
| m_delaySamples = SIZE - mixxx::kEngineChannelCount; |
There was a problem hiding this comment.
The original code just performs a. copy.
(Delay = 0) in that case. Since we are anyway In an anti crash backup path, I find mind, I just want to pint it out.
There was a problem hiding this comment.
I agree, that's right. This PR just offers another point of view for the problem, that the wrong delay values can be clamped in the setter. This version should be anti-crash too. Of course, I will be okay, if the PR won't be merged. It is just proposed to use a more similar approach as in EngineDelay or the future used EngineEffectsDelay.
| // When mixxx will support other channel count variants than stereo, | ||
| // the kMaxDelay has to be divisible by the number of channels. | ||
| // Otherwise, channels may be swapped. | ||
| VERIFY_OR_DEBUG_ASSERT(m_delaySamples <= SIZE - mixxx::kEngineChannelCount) { |
There was a problem hiding this comment.
Is this condition correct, or do we need to substract the engine buffer size?
There was a problem hiding this comment.
Can you please explain more about why you think of substracting the engine buffer size instead and which "engine buffer size" you think of? The size of the input buffer? The reason, why the number of channels is subtracted is, that the delay size can't be equal to the size of the delay buffer (m_buf). It can be smaller and subtracting the number of channels ensures, that the channels won't be swapped between. IMO the assertion shouldn't go wrong as original, because it doesn't allow the delay values greater than possible (greater or equal to buffer size), so the m_delaySamples values from the examples wouldn't be allowed.
There was a problem hiding this comment.
Ah sorry, I have misread the code. In case of 0 delay, you but one frame into the buffer and read it back immediately.
That is why - mixxx::kEngineChannelCount is sufficient.
There was a problem hiding this comment.
No problem. Perfect.
This commit moves the correctness evaluation of the delay value into a setter and the set value is directly checked instead of calculated index values.
c792d31 to
855be72
Compare
This commit adds two assertions. The first one is a static_assert which tests if the templated SIZE is divisible by the number of channels. The second one is VERIFY_OR_DEBUG_ASSERT in EngineFilterDelay::setDelay. It tests if the number of delay samples is divisible by the number of channels. If the delay size is not aligned, the unaligned samples are cropped (rounded to the previous multiple of the number of channel count).
|
Thank you very much for merging. |
|
thanks for taking care @daschuer |
This PR moves the delay value checking into a setter in
EngineFilterDelay. The reason, why the delay value checking should be updated is, that the previous version allows working with a huge not acceptable delay, but which is after inner calculation allowed due to it is a k-multiple (on the left side of the modulo operator) of the delay buffer size (on the right side of the modulo operator). Three examples for which is in previous version delay allowed, and should not be:Let's work with the used size of the delay buffer as
SIZE = 3300.The first example works with delay size as 4*3300 = 13200
int delaySourcePos = (m_delayPos + SIZE - m_delaySamples) % SIZE = (0 + 3300 - 13200) % 3300 = -9900 % 3300 = 0The second example works with a huge delay, but which is the same as
m_delayPos + SIZE.int delaySourcePos = (m_delayPos + SIZE - m_delaySamples) % SIZE = (2426 + 3300 - 5726) % 3300 = 0 % 3300 = 0The third example works with the situation similar to 1, anyway, it shows that the position has not to be zero.
int delaySourcePos = (m_delayPos + SIZE - m_delaySamples) % SIZE = (1024 + 3300 - 7624) % 3300 = -3300 % 3300 = 0Based on the delay value checking in the setter, the situation, that the previous used
VERIFY_OR_DEBUG_ASSERTwill be violated should not occur.