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

Don't process EQ until the user actually changes an EQ value. #227

Merged
merged 2 commits into from
Apr 12, 2014

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Apr 10, 2014

Saves CPU for our many users who never touch the controls.
TESTED=hand tested, didn't hear popping when the changeover happens, also spat out debug output to confirm it did what I thought

CPU for our many users who never touch the controls.
TESTED=hand tested, didn't hear popping when the changeover happens
@daschuer
Copy link
Member

This patch is intended to gain a lower latency on poor hardware, right?

On the fist thought, I found this patch useful, but this could be wrong.

In a reliable real-time application, the CPU usage should be very deterministic.
Apart from the power consumption, it is ideal if every audio-callback call requires the same CPU.
regardless of the system load. If a system is configured four decks, it should be possible to use them
without dropouts the whole night long.

With this patch we may have the situation, that Mixxx starts to play junk due to dropouts to the audience after touching a EQ knob. We have the same situation with keylock and effects and this is a really bad situation. Unfortunately I don't have a handy solution here.

But maybe we have an idea to get around this issue.

An option to permanent disable the EQs maybe help here but this way we cannot use the "saved" CPU elsewhere. :-/

Is the user on a poor hardware able to think this way: "OK, all EQs are at zero, so I can enable keylock at one Deck"

We may need a CPU manager. Something like a compressor for time. ...

@@ -37,6 +37,7 @@ EngineFilterBlock::EngineFilterBlock(const char* group)
ilowFreq = 0;
ihighFreq = 0;
blofi = false;
m_eqNeverTouched = true;
Copy link
Member

Choose a reason for hiding this comment

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

If you like you can move this to the initializer list after ":"

@ywwg
Copy link
Member Author

ywwg commented Apr 10, 2014

This patch is intended to gain a lower latency on poor hardware, right?

Not just that. We have many users who use Mixxx for internet streaming, or even just music playback. They never touch the EQ knobs, so it's a waste to do that calculation. Especially since EQ takes up a lot of CPU time, it's nice to have Mixxx use less CPU. And it's not just a matter of dropouts, I'm sure users will appreciate improved battery life that comes with wasting less CPU.

As for the question of a DJ experiencing dropouts only after touching EQ, this would seem to imply a DJ who has never tested their system to see if it's working. And if they are so close to CPU max that turning on EQ pushes them over the edge, they will probably get dropouts anyway. Also, while we're turning on EQ to push CPU usage up to "normal," should we also load 4 effects and crank them to the max? Should we also start writing the mix do disk to stress the IO system? And streaming it? There are lots of operations one can do that push CPU usage up, and I think it's a good thing that our CPU usage scales with how heavily the application is being used.

@esbrandt
Copy link
Contributor

An option to permanent disable the EQs maybe help here

Isn't that #154 was about?

@ywwg
Copy link
Member Author

ywwg commented Apr 10, 2014

Isn't that #154 was about?

yes, that already exists. but lots of people won't know to set that option.

@daschuer
Copy link
Member

OK, I think we can solve the issue in this PR. A CPU widget will help a bit https://bugs.launchpad.net/mixxx/+bug/89298.

Can you add a note to the hardware preferences, that the EQ knobs / key locks / and effects may effect latency.

This patch could be very useful for "bit-perfect" playback.
What is the reason that it works only if the EQ is never touched?
Is there a way to visualize the EQ bypass state?

@ywwg
Copy link
Member Author

ywwg commented Apr 10, 2014

What is the reason that it works only if the EQ is never touched?

One other suggestion people have made is, why not disable eqs if all the EQ knobs are back at 1.0? The problem happens when the user sweeps an EQ from X to Y, where X<1<Y. At some point the value will be 1.0, and for that single buffer the EQs will be disabled. This ends up causing audio distortion, since audio with EQ's at 1.0 is not the same as passthrough audio. So once the user touches the EQs, it's best just to leave it on all the time rather than try to guess when they've stopped using them.

I could imagine extending this logic to "if the playback is stopped and all the EQs are 1.0, don't process EQ again until the user touches a knob." But the purpose of this PR is to be very low-impact and make a difference for a certain class of user that we have: people who never touch the EQs ever. There's no reason we can't make this more full-featured later.

@ywwg
Copy link
Member Author

ywwg commented Apr 10, 2014

(and no, there is currently no feedback regarding EQ disabling. If we ever write "disabled widget" support, that would be how we communicate the feedback. For now, EQ disabling is a hidden CO-only option that most people won't find, but is useful for those bug reporters who complain about our EQs)

@ywwg
Copy link
Member Author

ywwg commented Apr 11, 2014

any more comments?

@daschuer
Copy link
Member

I am still torn between improvement and regression.
From this point of view we can merge it because we have 75 % pro votes ;-)
I third opinion would be helpful. (@esbrandt: merge?)

I will try to summarize:
We have this benefits for EQs at zero and never touched
a. Improved latency
b. improved sound quality

We have this drawbacks

  1. unexpected CPU load once EQs are touched
  2. unexpected sound quality drop once EQs are touched
  3. Now way to return to untouched state
  4. patch will not work for Controller users where it is hard to center EQ knobs
  5. No visual feedback in the GUI

It would be really nice, if we can solve some of these drawbacks before merge.
Ideas:

for 1. We should encourage pure hardware and low latency users to disable EG entirely.
This can be done by help text in hardware preferences about how to tweak latency and hints about the effects when using keylock effects and EQ

for 2, 3 and 4 This can be done by an implicit D/W mix:

  • all EQs at 1 = dry
  • changing to wet with the maximum distance of all EQ knobs.
    like D/W = max( abs(high), abs(mid), abs(low))
  • The same is done in a RGBW lamp where white is used to achieve a bright white or in color printers with black ink.

for 5. We may also add "direct sound" control, this can be uses as indicator for the direct position.

@rryan
Copy link
Member

rryan commented Apr 12, 2014

I think this is fine as is -- @ywwg is right if the user doesn't find out that using features cost CPU then they will eventually when they do a pitch adjust.

@ywwg
Copy link
Member Author

ywwg commented Apr 12, 2014

unexpected sound quality drop once EQs are touched

Note, it's not that the sound quality drops, it's just slightly different. I'm not sure it's possible to end up with a 100% sample-for-sample recreation of the sound after decomposing and then recomposing the signal. So I guess technically it's a "drop" after all, but I don't think it sounds "worse."

I am going to merge as-is. I know what you mean about controllers with hard-to-center knobs, I have one. If I really wanted to center them I'd right click them.

ywwg added a commit that referenced this pull request Apr 12, 2014
Don't process EQ until the user actually changes an EQ value.
@ywwg ywwg merged commit 74689ea into mixxxdj:master Apr 12, 2014
m0dB pushed a commit to m0dB/mixxx that referenced this pull request Jan 21, 2024
news: Use Markdown and fix formatting of GSoC 2013 post
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants