Fix ANGLE/OpenGL swap buffer issues under Windows by JoergAtGithub · Pull Request #3367 · mixxxdj/mixxx

JoergAtGithub · 2020-11-24T19:25:20Z

This fixes the bugs https://bugs.launchpad.net/mixxx/+bug/1895427 and https://bugs.launchpad.net/mixxx/+bug/1872172 under the OpenGL emulation ANGLE.

I tested this fix with ANGLE the angle library, that comes with 2.3-j00019-x64-release-fastbuild-static-55e94982-minimal as part of the Qt 5.14.2.
It also works if I set QT_OPENGL to desktop to use the native OpenGL driver of my Intel HD Graphics 3000 GPU.

I can't test the behaviour on other platforms.

Furthermore it fixes issue https://bugs.launchpad.net/mixxx/+bug/1905123 by adding the missing ANGLE DLLs to the windows installer.

Be-ing · 2020-11-24T19:29:23Z

Yay! I'm glad you found a solution that doesn't require any hacks overriding the ANGLE version included in Qt.

Be-ing · 2020-11-24T19:36:46Z

We still need to copy the ANGLE DLLs into the installer right? Can you add install commands to CMakeLists.txt to do that?

daschuer

Thank you for do all the hard debugging and discover this.
I will test this on Linux tomorrow.

daschuer · 2020-11-24T22:25:34Z

                }
                QGLWidget* glw = qobject_cast<QGLWidget*>(pWaveformWidget->getWidget());
                if (glw != nullptr) {
+                    glw->makeCurrent();


Does it still work when guarding it with:
if (QGLContext::currentContext() != context()) {

What should be context() in this statement? I didn't get the context here;-)

ups, glw->context()

This code seems also work without failures:

Combine it with the if condition above: if (glw != nullptr && glw->context() != QGLContext::curentContext()) {

nevermind, that isn't logically equivalent

@daschuer does glw->makeCurrent() have any performance penalty if the context is already current? Is the check for glw->context() != QGLContext::currentContext() needed?

The problem seems to be, that the openGL context is sometimes not set to the correct thread: https://doc.qt.io/qt-5/qopenglcontext.html#makeCurrent
In this cases swapBuffers fails, but swapBuffers has to be executed always.

@daschuer does glw->makeCurrent() have any performance penalty if the context is already current? Is the check for glw->context() != QGLContext::currentContext() needed?

makeCurrent() is expensive. We have the check everywhere else for some reasons.

Maybe this is done internally anyway. in this case we can omit the check.
I need to debug into the code to be sure.

The other puzzling question is why it works with real hardware. Maybe we are doing something wrong that steals the context from the thread in the Angle case.

Maybe this is done internally anyway.

I checked. It is not. We should do the check before calling makeCurrent.
https://code.woboq.org/qt5/qtbase/src/opengl/qgl.cpp.html#_ZN10QGLContext11makeCurrentEv

Be-ing · 2020-11-24T22:46:18Z

Please merge or rebase onto 2.3 to include #3368. I merged these two in my own branch to try to get GitHub Actions to build an artifact to test. However, the new install command in CMakeLists.txt is breaking cpack:
https://github.com/Be-ing/mixxx/runs/1450285227

CPack: Create package using WIX
CPack: Install projects
CPack: - Install project: mixxx
CMake Error at D:/a/mixxx/mixxx/cmake_build/cmake_install.cmake:56 (file):
  file INSTALL cannot find "/bin/libEGL.dll".


CPack Error: Error when generating package: mixxx

uklotzde · 2020-11-24T22:49:50Z

Rebase and stash the commits.

…l_spinnies_bug

Be-ing · 2020-11-24T23:04:36Z

I searched CMakeLists.txt for egl and found this interesting bit:

    target_link_libraries(mixxx-lib PUBLIC
      # Pulled from qt-4.8.2-source\mkspecs\win32-msvc2010\qmake.conf
      # QtCore
      kernel32
      user32      # QtGui, QtOpenGL, libHSS1394
      shell32
      uuid
      ole32       # QtGui,
      advapi32    # QtGui, portaudio, portmidi
      ws2_32      # QtGui, QtNetwork, libshout
      # QtGui
      gdi32       # QtOpenGL, libshout
      comdlg32
      oleaut32
      imm32
      winmm
      winspool
      # QtOpenGL
      glu32
      opengl32

      # QtNetwork openssl-linked
      crypt32

      # New libraries required by Qt5.
      dwmapi      # qtwindows
      iphlpapi    # qt5network
      #libEGL      # qt5opengl
      #libGLESv2   # qt5opengl
      mpr         # qt5core
      netapi32    # qt5core
      userenv     # qt5core
      uxtheme     # ?
      version     # ?
      wtsapi32    # ?

      # NOTE(rryan): If you are adding a plugin here, you must also
      # update src/mixxxapplication.cpp to define a Q_IMPORT_PLUGIN
      # for it. Not all imageformats plugins are built as .libs when
      # building Qt statically on Windows. Check the build environment
      # to see exactly what's available as a standalone .lib vs linked
      # into Qt .libs by default.

      # iconengines plugins
      Qt5::QSvgIconPlugin

      # imageformats plugins
      Qt5::QGifPlugin
      Qt5::QICOPlugin
      Qt5::QJpegPlugin
      Qt5::QSvgPlugin
      Qt5::QTgaPlugin

      # platform plugins (new in Qt5 for Windows)
      Qt5::QWindowsIntegrationPlugin

      # styles (new in Qt5 for Windows)
      Qt5::QWindowsVistaStylePlugin

      # sqldrivers (new in Qt5? or did we just start enabling them)
      Qt5::QSQLiteDriverPlugin
    )

Should we be using target_link_libraries to dynamically link libEGL and libGLESv2 instead of manually installing the DLLs? @Holzhaus what do you think?

Holzhaus · 2020-11-24T23:41:41Z

Should we be using target_link_libraries to dynamically link libEGL and libGLESv2 instead of manually installing the DLLs? @Holzhaus what do you think?

I honestly have no idea. Are they always present on windows (i.e. system libraries)?

Be-ing · 2020-11-24T23:42:40Z

No, libEGL.dll and libGLESv2.dll come from ANGLE which is included in Qt. They are hacks around the OS not properly supporting OpenGL.

Holzhaus · 2020-11-24T23:54:37Z

If we link them dynamically without shipping them, won't it cause issues because the windows loader will fail to locate the DLLs? I have no idea how all that stuff works on windows. If you think it's an improvement, try it.

Be-ing · 2020-11-24T23:57:27Z

cpack failed again:

Run cpack -G WIX
CPack: Create package using WIX
CPack: Install projects
CPack: - Install project: mixxx
CMake Error at D:/a/mixxx/mixxx/cmake_build/cmake_install.cmake:56 (file):
  file INSTALL cannot find "/../../../bin/libEGL.dll".

It seems Qt5_DIR is not initialized at the point where the install call is made. I think it is initialized by find_package(Qt5 lower down in CMakeLists.txt.

I think it is worth a try uncommenting libEGL and libGLESv2 from the target_link_libraries(mixxx-lib PUBLIC call. It might be necessary to get rid of the lib prefix.

Be-ing · 2020-11-24T23:58:23Z

If we link them dynamically without shipping them

I am thinking cpack might automatically include them in the MSI installer if they are linked by cmake. I could be wrong about that though.

Holzhaus · 2020-11-25T00:03:17Z

🤷

Be-ing · 2020-11-25T18:15:31Z

Should we be using target_link_libraries to dynamically link libEGL and libGLESv2 instead of manually installing the DLLs?

I tested this and it does not work. Linking fails.

Other approach to add ANGLE DLLs to cmake

Be-ing · 2020-11-25T19:26:37Z

I fixed the CMakeLists.txt and cleaned up the Git history in #3370.

Be-ing · 2020-11-25T19:26:54Z

Thank you @JoergAtGithub for working on this!

Fix OpenGL swap buffer issues

478094a

github-actions Bot added the ui label Nov 24, 2020

Be-ing requested a review from daschuer November 24, 2020 19:28

Be-ing added the critical bug label Nov 24, 2020

Be-ing added this to the 2.3.0 milestone Nov 24, 2020

Add ANGLE DLLs to Windows installer

d393448

github-actions Bot added the build label Nov 24, 2020

Be-ing reviewed Nov 24, 2020

View reviewed changes

Comment thread CMakeLists.txt Outdated

Be-ing reviewed Nov 24, 2020

View reviewed changes

Comment thread CMakeLists.txt Outdated

Fixed spelling in comment

ecbbd1e

Be-ing reviewed Nov 24, 2020

View reviewed changes

Comment thread CMakeLists.txt Outdated

Fixed indentation

31926da

Be-ing reviewed Nov 24, 2020

View reviewed changes

Comment thread CMakeLists.txt Outdated

Indentation change to two spaces

f3ae684

daschuer reviewed Nov 24, 2020

View reviewed changes

JoergAtGithub added 2 commits November 24, 2020 23:51

Merge remote-tracking branch 'upstream/2.3' into fixing_windows_openg…

5922752

…l_spinnies_bug

Corrected ANGLE DLL path

8c28357

Guarded makeCurrent

6c149a1

Other approach to add ANGLE DLLs to cmake

Be-ing mentioned this pull request Nov 25, 2020

fix ANGLE OpenGL issues on Windows #3370

Merged

Be-ing closed this Nov 25, 2020

JoergAtGithub deleted the fixing_windows_opengl_spinnies_bug branch November 25, 2020 20:03

Uh oh!

Conversation

JoergAtGithub commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Nov 24, 2020

Uh oh!

Be-ing commented Nov 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing commented Nov 24, 2020

Uh oh!

uklotzde commented Nov 24, 2020

Uh oh!

Be-ing commented Nov 24, 2020

Uh oh!

Holzhaus commented Nov 24, 2020

Uh oh!

Be-ing commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Holzhaus commented Nov 24, 2020

Uh oh!

Be-ing commented Nov 24, 2020

Uh oh!

Be-ing commented Nov 24, 2020

Uh oh!

Holzhaus commented Nov 25, 2020

Uh oh!

Be-ing commented Nov 25, 2020

Uh oh!

Be-ing commented Nov 25, 2020

Uh oh!

Be-ing commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JoergAtGithub commented Nov 24, 2020 •

edited

Loading

Be-ing commented Nov 24, 2020 •

edited

Loading