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

add option to disable OpenGL & disable it by default for macOS #3403

Closed
wants to merge 4 commits into from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Dec 5, 2020

This required cleaning up some tangled code in WaveformWidgetFactory.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

Performance without OpenGL is much better on the old Macbook I am borrowing. I am able to achieve the same performance with the current 2.3 branch by using a CPU waveform renderer and hiding spinnies, so it doesn't seem to be necessary to make the SharedGLContext initialization in mixxx.cpp conditional on whether OpenGL is enabled. I think Mixxx can reload the skin to toggle OpenGL to reload spinnies instead of making the user reboot Mixxx.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

I think Mixxx can reload the skin to toggle OpenGL to reload spinnies instead of making the user reboot Mixxx.

Well, not on macOS. Sometimes this happens 😩

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   org.qt-project.QtGui          	0x000000010d48d6c9 QOpenGLContext::format() const + 9
1   org.qt-project.QtGui          	0x000000010d777f8d QOpenGLFunctions_2_1::initializeOpenGLFunctions() + 93
2   org.mixxx.mixxx               	0x000000010bb04165 GLSLWaveformRendererSignal::GLSLWaveformRendererSignal(WaveformWidgetRenderer*, bool) + 181

@Be-ing Be-ing force-pushed the opengl branch 2 times, most recently from 7e40c0f to 27a6551 Compare December 5, 2020 09:27
OpenGL is disabled by default on macOS, so do not show the spinny
plaeholders by default. There is no way to conditionally set default
skin settings differently for each OS.
@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

Running this branch on the old Macbook I'm using with an Intel Core i5 2557M 1.7 GHz dual core CPU, Mixxx is usable with OpenGL disabled and the framerate at 15. The knobs and sliders are responsive to mouse input. Of course the waveforms don't look so smooth at 15 FPS, but at least Mixxx is practically usable.

@foss-
Copy link
Contributor

foss- commented Dec 5, 2020

Spinny says "OpenGL disabled" so assuming that is the case. When playing a track on a 2017 MacBook Pro mixxx still goes to 100% CPU, even with waveform disabled.

@daschuer
Copy link
Member

daschuer commented Dec 5, 2020

Spinny says "OpenGL disabled" so assuming that is the case. When playing a track on a 2017 MacBook Pro mixxx still goes to 100% CPU, even with waveform disabled.

Which Audio buffer size do you use and which frame rate?
Is the library scanner working?
What is your CPU type? How many real and hyperthreading cores does the CPU have?
Are all cores at 100 %?

The waveforms are rendered once a track is loaded.
Please watch for the start Mixxx and watch out when exactly the CPU load rises.

You may also create a couple of back-traces during 100% CPU state.
Under GDB, brake Mixxx with Ctrl-C and than save the output of bt --all

Thank you.

@@ -18,7 +18,7 @@
<attribute persist="true" config_key="[Skin],show_8_hotcues">1</attribute>
<attribute persist="true" config_key="[Skin],show_intro_outro_cues">1</attribute>
<attribute persist="true" config_key="[Skin],show_coverart">1</attribute>
<attribute persist="true" config_key="[Skin],show_spinnies">1</attribute>
<attribute persist="true" config_key="[Skin],show_spinnies">0</attribute>
Copy link
Member

Choose a reason for hiding this comment

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

What is the reasoning for this change?
If you think it is an issue to show an empty Spinny in case of no OpenGL, we should always hide it in this case.
But that is only a one time issue for Mac users, while new Windows user will have hard times to discover the spinnies.

Copy link
Contributor Author

@Be-ing Be-ing Dec 5, 2020

Choose a reason for hiding this comment

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

If you think it is an issue to show an empty Spinny in case of no OpenGL, we should always hide it in this case.

I disagree. Then we would need to explain somewhere in the GUI that spinnies will be disabled without OpenGL. Users will be confused if spinnies disappear when updating Mixxx without any explanation and the toggles in the skin settings do not work. With this solution, users installing Mixxx for the first time will probably not notice anything. If they enable the spinnies in the skin settings, they will immediately see why they are not working rather than confusingly having no feedback.

new Windows user will have hard times to discover the spinnies.

I don't mind that.

m_lastActivity = mixxx::Time::elapsed();
if (!m_guiTickTimer.isActive()) {
m_guiTickTimer.start(m_renderFrequency);
#ifdef __APPLE__
Copy link
Member

Choose a reason for hiding this comment

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

This requires some comments.

Copy link
Member

Choose a reason for hiding this comment

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

Is

m_pGuiTick->process();

not reached?

I think it would be better to repair the instead of clutter every Widget with is workaround.
If this is not possible i think it would be cleaner to indicate by [Master]", "guiTickTime" = -1 that it is not working.
Than every user of this CO can check this easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comments are in the header already.

}
#else
emit update();
Copy link
Member

Choose a reason for hiding this comment

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

This is new now. Is it really required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried commenting it out. Yes, it is required.

Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to change the code for other platforms than MacOS here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise the widgets do not update. Try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose I could #ifdef the whole inputActivity methods instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, that would require many more #ifdefs. I am not going down that rabbit hole.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not really new. I only moved it into WidgetRenderTimer so each class that uses WidgetRenderTimer doesn't need to repeat the same #ifdefs.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

Spinny says "OpenGL disabled" so assuming that is the case. When playing a track on a 2017 MacBook Pro mixxx still goes to 100% CPU, even with waveform disabled.

Have you tried adjusting the framerate? Is the GUI responsive?

@JoergAtGithub
Copy link
Member

JoergAtGithub commented Dec 5, 2020

Spinny says "OpenGL disabled" so assuming that is the case. When playing a track on a 2017 MacBook Pro mixxx still goes to 100% CPU, even with waveform disabled.

Is this the same system, you mentiond in https://bugs.launchpad.net/mixxx/+bug/1906287 , where the early 2.3 beta builds run with OpenGL activated and 25% CPU load?
Can you do the reverse test and install the old 2.3 beta build again. Do you still have low CPU load with this build?

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

@JoergAtGithub can you confirm there are no regressions with this branch on Windows?

@JoergAtGithub
Copy link
Member

I tested the artifact from the GitHub build with Windows7. And noticed, that changeing the Waveform Overview Type now requires a skin reload. Shouldn't this only happen, when the OpenGL checkbox is toggled?

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

I didn't change anything relating to the overview waveforms on this branch.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

I think we should merge this even if it isn't an effective workaround for all performance issues. It is still useful to have a single, easy to use checkbox to toggle OpenGL so users can test the impact easily. Explaining "you must pick a CPU waveform and hide spinnies" is more complicated and introduces room for error/miscommunication.

@ronso0
Copy link
Member

ronso0 commented Dec 5, 2020

I tested the artifact from the GitHub build with Windows7. And noticed, that changeing the Waveform Overview Type now requires a skin reload. Shouldn't this only happen, when the OpenGL checkbox is toggled?

I think when my test PR for removing skin restart was merged, no one remembered to revert the other commits from #3373, too.
I'm probably afk for a few more days, so I'd appreciate if someone could take care of that.

(damn Close button)

@ronso0 ronso0 closed this Dec 5, 2020
@ronso0 ronso0 reopened this Dec 5, 2020
@foss-
Copy link
Contributor

foss- commented Dec 5, 2020

I retested. not sure why this did not show improved CPU usage initially. I have to correct that statement. CPU usage with RGB waveform and OpenGL disabled mixxx is around 60% when playing a track. Goes to ~80% when playing two tracks. And at ~30-40% sitting idle.

Mouse usage

  • Faders respond great
  • Knobs respond poorly: using the trackpad it is extremely difficult to get the knob to the desired value. It tilts between 0-100% much too quickly

Controller usage

  • can't test as no controller mapping is currently shown for xone k2

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

Knobs respond poor: using the trackpad it is extremely difficult to get the knob to the desired value. It tilts between 0-100% much too quickly

That's great. It's much better than the opposite problem. Trying to adjust the sensitivity to mouse input in Mixxx is likely futile considering the wide variety of hardware and software configurations, even within the relatively restricted range of Mac hardware.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 5, 2020

@JoergAtGithub how is the performance with this on Windows with and without OpenGL?

@JoergAtGithub
Copy link
Member

The overall UI performance seems not to be impacted by OpenGL, only the waveform framerate. With my underpowered Intel GPU, this is acceptable, but not good. Native OpenGL is slightly faster than ANGLE in my environment. The lowest framerate show the (QT GL) waveforms.

@poelzi
Copy link
Contributor

poelzi commented Dec 6, 2020

On my linux with "NVIDIA Corporation GM108M [GeForce 940MX] " card using optimus I would consider the performance ok. If mixxx runs on the WQXGA monitor I get between 35-40 fps but quite some drops. 30 fps is relative stable. I use my #3153 renderer (glsl)

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 6, 2020

This shouldn't have any impact on systems where OpenGL works (Windows and Linux with functioning drivers) if you keep OpenGL enabled.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 7, 2020

Closing this now that I have identified rendering WSpinny as the big performance problem.

@Be-ing Be-ing closed this Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants