-
-
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
Smoothly ramp changes in gain. #39
Conversation
// buffers must be the same length. pDest may be an alias of the source | ||
// buffers. pSrc1 is the buffer that is fading out, pSrc2 is the buffer | ||
// that is fading in. | ||
static void crossfadeBuffers(CSAMPLE* pDest, |
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.
Maybe call it linearCrossfadeBuffers since it does linear crossfades.
Thank you for the fix. I just want to clarify that the result is Audio-buffer-size dependent, right? |
Yea, the crossfade step is dependent on the audio buffer size so depending on the magnitude of the change in gain we could still get clicks (as we talked about on the bug report). |
The worst-case scenario I can imagine is 1ms latency at 44.1Khz, which is 44.1 samples per buffer. Even having that amount of space to interpolate is better than nothing. But also, as the buffer becomes smaller the delta gain will also become smaller because the user has less time to make the change. (Could a user twist a knob from 64 to 0 in 1ms? unlikely). So in practice I'd be surprised if it were possible to cause clicks. |
An eq kill button or resetting an eq knob could take it from max value to 0 in one callback. There's really no perfect solution :). This one is a good one. |
(the most recent changes are untested, but it builds. Do not merge until I've actually done testing) |
the crossfader and gain knob additions are total fails. I will fix them. |
Ok pregain and master are working now, but headphone is tougher because it has a bunch of moving pieces that aren't all together (balance, gain, master mix) |
OK to merge? |
private: | ||
double m_dVolume, m_dLeftGain, m_dCenterGain, m_dRightGain; | ||
QMap<ChannelInfo*, double> m_cache; |
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.
Rather than use a QMap, can you keep the cached value in ChannelInfo? We already have ChannelInfo. It may be ugly but it avoids using a QMap in the callback.
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.
Hm, the problem with that is it becomes difficult to do the comparison of the current gains to the previous gains to see if we need to do a crossfade. With the map, I can see if the two maps are equal. But without it, I'll have to iterate through the list of pchannels.
notes addressed other than caching suggestion (see comment). (and I will squash some commits before merging so please let me do the final merge) |
@@ -397,6 +403,20 @@ void EngineMaster::process(const CSAMPLE *, const CSAMPLE *pOut, const int iBuff | |||
// Perform the master mix | |||
mixChannels(masterOutput, maxChannels, m_pMaster, iBufferSize, &m_masterGain); | |||
|
|||
// Avoid soundwave discontinuties by interpolating from the old gains to new. | |||
if (!m_masterGain.compare(m_prevMasterGain)) { | |||
// We are reusing p_pPrevGainBuffer from above, but its data isn't needed |
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.
out of date comment
…gains. address review notes
abandon headphone buffer crossfading for now, it's got too many moving pieces
…cache every time.
…gains. address review notes
abandon headphone buffer crossfading for now, it's got too many moving pieces
…cache every time.
Conflicts: src/engine/enginemaster.cpp src/engine/enginemaster.h src/sampleutil.cpp src/sampleutil.h
Rebased, merged in RJ's changes. |
I will do some testing on this tonight before I commit the merge. |
Cool -- thanks! There are still those outdated comments you marked to fix. We should do some profiling. mixChannels is not an engine hot-spot (at least, it's nowhere near as hot as the EQs) -- it takes tens of microseconds last time I checked. But it would still be interesting to know the change ramping causes on various hardware. |
performance testing: I loaded a track, started playing it, and then wiggled the mid eq really fast with the mouse. trunk: crossfade: I think it's better? I'm surprised about that. |
LGTM and stuff |
Ok I tracked down the first commit that makes the enginemaster test fail, it is 6d5d02f After this enginemaster changes the channel buffer. When you later also merge in all the changes of rryan everytest starts to fail. I guess it has something to do with the changes you made in the gain calculation but I'm not sure since I don't understand the code here. |
Add lp:1846966 to 2.2.3 changelog
https://bugs.launchpad.net/mixxx/+bug/854082
Add
copyWithRampingGain
,applyRampingGain
andaddWithRampingGain
methods toSampleUtil
and use them to ramp changes in gain in various places:Still remaining to be done: