Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions src/mixxxmainwindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
#endif

#ifdef MIXXX_USE_QOPENGL
#include <QGuiApplication>

#include "widget/tooltipqopengl.h"
#include "widget/winitialglwidget.h"
#endif
Expand Down Expand Up @@ -156,22 +158,35 @@ void MixxxMainWindow::initializeQOpenGL() {
// QGLFormat::hasOpenGL() has been removed.
if (!CmdlineArgs::Instance().getSafeMode() && QGLFormat::hasOpenGL()) {
#else
if (!CmdlineArgs::Instance().getSafeMode()) {
// With EGLFS there is always exactly one native window and one EGL window surface
// OpenGL windows cannot be embedded into our QWidgets main window we already have.
// https://doc.qt.io/qt-6/embedded-linux.html
bool isEglfs = QGuiApplication::platformName() == "eglfs";
Comment thread
ywwg marked this conversation as resolved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we know if this string is normalised? Wondering if somebody passes QT_QPA_PLATFORM=EGLFS, whether the platform name would remain lower cased or ignored, of if we should normalise the output. I am under the impression it should be the doc isn't completely clear about this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

https://doc.qt.io/qt-6/qguiapplication.html#platformName-prop only one spelling is mentioned. I think we can trust it.
I will add a comment.


if (!CmdlineArgs::Instance().getSafeMode() && !isEglfs) {
#endif
QOpenGLContext context;
context.setFormat(WaveformWidgetFactory::getSurfaceFormat(m_pCoreServices->getSettings()));
if (context.create()) {
std::pair version = context.format().version();
Comment thread
JoergAtGithub marked this conversation as resolved.
qDebug().noquote()
<< "QOpenGLContext created:"
<< QGuiApplication::platformName()
<< context.format().renderableType()
<< QString("V%1.%2").arg(QString::number(version.first),
QString::number(version.second))
<< context.format().profile();
// This widget and its QOpenGLWindow will be used to query QOpenGL
// information (version, driver, etc) in WaveformWidgetFactory.
// The "SharedGLContext" terminology here doesn't really apply,
// but allows us to take advantage of the existing classes.
WInitialGLWidget* widget = new WInitialGLWidget(this);
widget->setGeometry(QRect(0, 0, 3, 3));
SharedGLContext::setWidget(widget);
WInitialGLWidget* pWidget = new WInitialGLWidget(this);
Comment thread
daschuer marked this conversation as resolved.
Outdated
pWidget->setGeometry(QRect(0, 0, 3, 3));
SharedGLContext::setWidget(pWidget);
// When the widget's QOpenGLWindow has been initialized, we continue
// with the actual initialization
connect(widget, &WInitialGLWidget::onInitialized, this, &MixxxMainWindow::initialize);
widget->show();
connect(pWidget, &WInitialGLWidget::onInitialized, this, &MixxxMainWindow::initialize);
pWidget->show();
return;
}
qDebug() << "QOpenGLContext::create() failed";
Expand Down
38 changes: 27 additions & 11 deletions src/waveform/waveformwidgetfactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "waveform/waveform.h"

#ifdef MIXXX_USE_QOPENGL
#include <QGuiApplication>
#include <QOpenGLShaderProgram>
#include <QOpenGLWindow>
#else
Expand Down Expand Up @@ -185,17 +186,30 @@ WaveformWidgetFactory::WaveformWidgetFactory()

m_openGLShaderAvailable = QOpenGLShaderProgram::hasOpenGLShaderPrograms(pContext);

m_openGLVersion = pContext->isOpenGLES() ? "ES " : "";
// With EGLFS there is always exactly one native window and one EGL window surface
// OpenGL windows cannot be embedded into our QWidgets main window we already have.
// That's why m_openGlesAvailable is not set to true. TODO: use GL Widgets for all
// https://doc.qt.io/qt-6/embedded-linux.html
bool isEglfs = QGuiApplication::platformName() == "eglfs";

if (isEglfs) {
m_openGLVersion = "EGLFS ";
} else if (pContext->isOpenGLES()) {
m_openGLVersion = "ES ";
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This used to be init-ed to an empty string if not isOpenGLES. Not sure if there is a risk this strings need to be cleared? Perhaps a DEBUG_ASSERT is enough.

Suggested change
} else if (pContext->isOpenGLES()) {
m_openGLVersion = "ES ";
}
} else if (pContext->isOpenGLES()) {
m_openGLVersion = "ES ";
} else {
m_openGLVersion = "";
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We are here in the constructor, the constructor initializes the QString as empty. So nothing to do IMHO, except using QStringLiteral. I will also add a comment.

m_openGLVersion += majorVersion == 0 ? QString("None") : versionString;

// Qt5 requires at least OpenGL 2.1 or OpenGL ES 2.0
if (pContext->isOpenGLES()) {
if (majorVersion * 100 + minorVersion >= 200) {
m_openGlesAvailable = true;
}
} else {
if (majorVersion * 100 + minorVersion >= 201) {
m_openGlAvailable = true;
if (!isEglfs) {
// Qt5 requires at least OpenGL 2.1 or OpenGL ES 2.0
Comment thread
acolombier marked this conversation as resolved.
Outdated
if (pContext->isOpenGLES()) {
if (majorVersion * 100 + minorVersion >= 200) {
m_openGlesAvailable = true;
}
} else {
if (majorVersion * 100 + minorVersion >= 201) {
m_openGlAvailable = true;
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we merge this condition to help readability?

Suggested change
if (pContext->isOpenGLES()) {
if (majorVersion * 100 + minorVersion >= 200) {
m_openGlesAvailable = true;
}
} else {
if (majorVersion * 100 + minorVersion >= 201) {
m_openGlAvailable = true;
}
}
if (pContext->isOpenGLES() && majorVersion * 100 + minorVersion >= 200) {
m_openGlesAvailable = true;
} else if (majorVersion * 100 + minorVersion >= 201) {
m_openGlAvailable = true;
}

Also wondering if we need to set m_openGlesAvailable and m_openGlAvailable, such as

Suggested change
if (pContext->isOpenGLES()) {
if (majorVersion * 100 + minorVersion >= 200) {
m_openGlesAvailable = true;
}
} else {
if (majorVersion * 100 + minorVersion >= 201) {
m_openGlAvailable = true;
}
}
m_openGlesAvailable = pContext->isOpenGLES() && majorVersion * 100 + minorVersion >= 200;
m_openGlAvailable = !pContext->isOpenGLES() && majorVersion * 100 + minorVersion >= 201;

If not, could you please put a comment to highlight that these are expected to be set to false otherwise during another step?

}

Expand Down Expand Up @@ -1365,10 +1379,12 @@ QString WaveformWidgetAbstractHandle::getDisplayName(WaveformWidgetType::Type ty
}

// static
QSurfaceFormat WaveformWidgetFactory::getSurfaceFormat(UserSettingsPointer config) {
QSurfaceFormat WaveformWidgetFactory::getSurfaceFormat(UserSettingsPointer pConfig) {
// The first call should pass the config to set the vsync mode. Subsequent
// calls will use the value as set on the first call.
static const auto vsyncMode = config->getValue(kVSyncKey, 0);
static const VSyncThread::VSyncMode vsyncMode = pConfig
? pConfig->getValue(kVSyncKey, VSyncThread::ST_DEFAULT)
: VSyncThread::ST_DEFAULT;

QSurfaceFormat format;
// Qt5 requires at least OpenGL 2.1 or OpenGL ES 2.0, default is 2.0
Expand Down
2 changes: 1 addition & 1 deletion src/waveform/waveformwidgetfactory.h
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ class WaveformWidgetFactory : public QObject,
static int toUntilMarkTextHeightLimitIndex(float value);

/// Returns the desired surface format for the OpenGLWindow
static QSurfaceFormat getSurfaceFormat(UserSettingsPointer config = nullptr);
static QSurfaceFormat getSurfaceFormat(UserSettingsPointer pConfig = nullptr);

protected:
bool setWidgetType(
Expand Down