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.

Sign up for GitHub

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

Fix crash when hiding waveforms in Deere #14170

Merged
merged 6 commits into from
Jan 14, 2025
+87 −55
Merged

Fix crash when hiding waveforms in Deere #14170

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6fc3426
Add missing p prefix to pTexture
daschuer Jan 12, 2025
ac9ac48
Make MarkerGeometry a class with const getter
daschuer Jan 14, 2025
5f693d0
Don't use runtime asserts
daschuer Jan 14, 2025
5f82622
Return early if there is nothing to paint.
daschuer Jan 14, 2025
8ae7d2f
Improve variable names
daschuer Jan 14, 2025
4c368f4
Use VERIFY_OR_DEBUG_ASSERT() to check for valid images
daschuer Jan 14, 2025
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
37 changes: 22 additions & 15 deletions src/waveform/renderers/allshader/waveformrendermark.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,17 +97,17 @@ void allshader::WaveformRenderMark::initializeGL() {
}

void allshader::WaveformRenderMark::drawTexture(
const QMatrix4x4& matrix, float x, float y, QOpenGLTexture* texture) {
const QMatrix4x4& matrix, float x, float y, QOpenGLTexture* pTexture) {
const float devicePixelRatio = m_waveformRenderer->getDevicePixelRatio();
const float texx1 = 0.f;
const float texy1 = 0.f;
const float texx2 = 1.f;
const float texy2 = 1.f;

const float posx1 = x;
const float posx2 = x + static_cast<float>(texture->width() / devicePixelRatio);
const float posx2 = x + static_cast<float>(pTexture->width() / devicePixelRatio);
const float posy1 = y;
const float posy2 = y + static_cast<float>(texture->height() / devicePixelRatio);
const float posy2 = y + static_cast<float>(pTexture->height() / devicePixelRatio);

const float posarray[] = {posx1, posy1, posx2, posy1, posx1, posy2, posx2, posy2};
const float texarray[] = {texx1, texy1, texx2, texy1, texx1, texy2, texx2, texy2};
Expand All @@ -130,11 +130,11 @@ void allshader::WaveformRenderMark::drawTexture(

m_textureShader.setUniformValue(textureLocation, 0);

texture->bind();
pTexture->bind();

glDrawArrays(GL_TRIANGLE_STRIP, 0, 4);

texture->release();
pTexture->release();

m_textureShader.disableAttributeArray(positionLocation);
m_textureShader.disableAttributeArray(texcoordLocation);
Expand Down Expand Up @@ -231,6 +231,11 @@ void allshader::WaveformRenderMark::paintGL() {
static_cast<TextureGraphics*>(pMark->m_pGraphics.get())
->texture();

if (!pTexture->isCreated()) {
// This happens if the height is zero
continue;
}

const float currentMarkPoint =
std::round(
static_cast<float>(
Expand Down Expand Up @@ -383,20 +388,22 @@ void allshader::WaveformRenderMark::drawUntilMark(const QMatrix4x4& matrix, floa
// Note that in the legacy waveform widgets this is drawn directly
// in the WaveformWidgetRenderer itself. Doing it here is cleaner.
void allshader::WaveformRenderMark::updatePlayPosMarkTexture() {
float imgwidth;
float imgheight;
const float imgheight = m_waveformRenderer->getBreadth();
const float imgwidth = 11.f;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const float imgheight = m_waveformRenderer->getBreadth();
const float imgwidth = 11.f;
const float imgHeight = m_waveformRenderer->getBreadth();
const float imgWidth = 11.f;

and where used, of course.


const float height = m_waveformRenderer->getBreadth();
const float devicePixelRatio = m_waveformRenderer->getDevicePixelRatio();
if (imgheight == 0.0f) {
return;
}

const float devicePixelRatio = m_waveformRenderer->getDevicePixelRatio();
const float lineX = 5.5f;

imgwidth = 11.f;
imgheight = height;

QImage image(static_cast<int>(imgwidth * devicePixelRatio),
static_cast<int>(imgheight * devicePixelRatio),
QImage::Format_ARGB32_Premultiplied);
if (image.isNull()) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

imgWidth is < 0 and there is already a check for imgHeight != 0 above, so this can only be NULL if devicePixelRatio is 0.
Can this ever happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

When looking at the Qt source, there are other reasons like out of memory or such. All can't probably not happen but this saves Mixxx from crash if it happen anyway. I think a
VERIFY_OR_DEBUG_ASSERT is a good choice.

image.setDevicePixelRatio(devicePixelRatio);
image.fill(QColor(0, 0, 0, 0).rgba());

Expand All @@ -417,8 +424,8 @@ void allshader::WaveformRenderMark::updatePlayPosMarkTexture() {
// lines next to playpos
// Note: don't draw lines where they would overlap the triangles,
// otherwise both translucent strokes add up to a darker tone.
painter.drawLine(QLineF(lineX + 1.f, 4.f, lineX + 1.f, height));
painter.drawLine(QLineF(lineX - 1.f, 4.f, lineX - 1.f, height));
painter.drawLine(QLineF(lineX + 1.f, 4.f, lineX + 1.f, imgheight));
painter.drawLine(QLineF(lineX - 1.f, 4.f, lineX - 1.f, imgheight));

// triangle at top edge
// Increase line/waveform contrast
Expand All @@ -433,7 +440,7 @@ void allshader::WaveformRenderMark::updatePlayPosMarkTexture() {
painter.setPen(fgColor);
painter.setOpacity(1.0);
// play position line
painter.drawLine(QLineF(lineX, 0.f, lineX, height));
painter.drawLine(QLineF(lineX, 0.f, lineX, imgheight));
// triangle at top edge
{
QPointF baseL = QPointF(lineX - 4.f, 0.f);
Expand Down
2 changes: 1 addition & 1 deletion src/waveform/renderers/allshader/waveformrendermark.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class allshader::WaveformRenderMark : public ::WaveformRenderMarkBase,
QPointF p3);

void drawMark(const QMatrix4x4& matrix, const QRectF& rect, QColor color);
void drawTexture(const QMatrix4x4& matrix, float x, float y, QOpenGLTexture* texture);
void drawTexture(const QMatrix4x4& matrix, float x, float y, QOpenGLTexture* pTexture);
void updateUntilMark(double playPosition, double markerPosition);
void drawUntilMark(const QMatrix4x4& matrix, float x);
float getMaxHeightForText(float proportion) const;
Expand Down
73 changes: 49 additions & 24 deletions src/waveform/renderers/waveformmark.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,13 +204,8 @@ bool WaveformMark::contains(QPoint point, Qt::Orientation orientation) const {

// Helper struct to calculate the geometry and fontsize needed by generateImage
// to draw the label and text
struct MarkerGeometry {
bool m_isSymbol; // is the label normal text or a single symbol (e.g. open circle arrow)
QFont m_font;
QRectF m_contentRect;
QRectF m_labelRect;
QSizeF m_imageSize;

class MarkerGeometry {
public:
MarkerGeometry(const QString& label,
bool useIcon,
Qt::Alignment align,
Expand Down Expand Up @@ -314,10 +309,37 @@ struct MarkerGeometry {
return QSize{static_cast<int>(m_imageSize.width() * devicePixelRatio),
static_cast<int>(m_imageSize.height() * devicePixelRatio)};
}

const QFont font() const {
return m_font;
}

const QRectF& contentRect() const {
return m_contentRect;
}

const QRectF& labelRect() const {
return m_labelRect;
}

const QSizeF& imageSize() const {
return m_imageSize;
}

private:
bool m_isSymbol; // is the label normal text or a single symbol (e.g. open circle arrow)
QFont m_font;
QRectF m_contentRect;
QRectF m_labelRect;
QSizeF m_imageSize;
};

QImage WaveformMark::generateImage(float devicePixelRatio) {
assert(needsImageUpdate());
DEBUG_ASSERT(needsImageUpdate());
Copy link
Member

Choose a reason for hiding this comment

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

VERIFY_OR_DEBUG_ASSERT ?
IIUC all below would be a no-op otherwise, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but we assert that it never happens and when it happens, it does no harm.


if (m_breadth == 0.0f) {
return {};
}

// Load the pixmap from file.
// If that succeeds loading the text and stroke is skipped.
Expand Down Expand Up @@ -355,11 +377,14 @@ QImage WaveformMark::generateImage(float devicePixelRatio) {
// Determine drawing geometries
const MarkerGeometry markerGeometry{label, useIcon, m_align, m_breadth, m_level};

m_label.setAreaRect(markerGeometry.m_labelRect);
m_label.setAreaRect(markerGeometry.labelRect());

// Create the image
QImage image{markerGeometry.getImageSize(devicePixelRatio),
QImage::Format_ARGB32_Premultiplied};
if (image.isNull()) {
return image;
}
Copy link
Member

Choose a reason for hiding this comment

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

how can that happen?
There is already the m_breadth == 0.0f above.

image.setDevicePixelRatio(devicePixelRatio);

// Fill with transparent pixels
Expand All @@ -374,53 +399,53 @@ QImage WaveformMark::generateImage(float devicePixelRatio) {
painter.setWorldMatrixEnabled(false);

// Draw marker lines
const auto hcenter = markerGeometry.m_imageSize.width() / 2.f;
const auto hcenter = markerGeometry.imageSize().width() / 2.f;
m_linePosition = static_cast<float>(hcenter);

// Draw the center line
painter.setPen(fillColor());
painter.drawLine(QLineF(hcenter, 0.f, hcenter, markerGeometry.m_imageSize.height()));
painter.drawLine(QLineF(hcenter, 0.f, hcenter, markerGeometry.imageSize().height()));

painter.setPen(borderColor());
painter.drawLine(QLineF(hcenter - 1.f,
0.f,
hcenter - 1.f,
markerGeometry.m_imageSize.height()));
markerGeometry.imageSize().height()));
painter.drawLine(QLineF(hcenter + 1.f,
0.f,
hcenter + 1.f,
markerGeometry.m_imageSize.height()));
markerGeometry.imageSize().height()));

if (useIcon || label.length() != 0) {
painter.setPen(borderColor());

// Draw the label rounded rect with border
QPainterPath path;
path.addRoundedRect(markerGeometry.m_labelRect, 2.f, 2.f);
path.addRoundedRect(markerGeometry.labelRect(), 2.f, 2.f);
painter.fillPath(path, fillColor());
painter.drawPath(path);

// Center m_contentRect.width() and m_contentRect.height() inside m_labelRect
// and apply the offset x,y so the text ends up in the centered width,height.
QPointF pos(markerGeometry.m_labelRect.x() +
(markerGeometry.m_labelRect.width() -
markerGeometry.m_contentRect.width()) /
QPointF pos(markerGeometry.labelRect().x() +
(markerGeometry.labelRect().width() -
markerGeometry.contentRect().width()) /
2.f -
markerGeometry.m_contentRect.x(),
markerGeometry.m_labelRect.y() +
(markerGeometry.m_labelRect.height() -
markerGeometry.m_contentRect.height()) /
markerGeometry.contentRect().x(),
markerGeometry.labelRect().y() +
(markerGeometry.labelRect().height() -
markerGeometry.contentRect().height()) /
2.f -
markerGeometry.m_contentRect.y());
markerGeometry.contentRect().y());

if (useIcon) {
QSvgRenderer svgRenderer(m_iconPath);
svgRenderer.render(&painter, QRectF(pos, markerGeometry.m_contentRect.size()));
svgRenderer.render(&painter, QRectF(pos, markerGeometry.contentRect().size()));
} else {
// Draw the text
painter.setBrush(Qt::transparent);
painter.setPen(labelColor());
painter.setFont(markerGeometry.m_font);
painter.setFont(markerGeometry.font());

painter.drawText(pos, label);
}
Expand Down
10 changes: 5 additions & 5 deletions src/widget/wspinnyglsl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,14 +171,14 @@ void WSpinnyGLSL::initializeGL() {
m_vinylQualityShader.init();
}

void WSpinnyGLSL::drawTexture(QOpenGLTexture* texture) {
void WSpinnyGLSL::drawTexture(QOpenGLTexture* pTexture) {
const float texx1 = 0.f;
const float texy1 = 1.0;
const float texx2 = 1.f;
const float texy2 = 0.f;

const float tw = texture->width();
const float th = texture->height();
const float tw = pTexture->width();
const float th = pTexture->height();

// fill centered
const float posx2 = tw >= th ? 1.f : tw / th;
Expand All @@ -197,11 +197,11 @@ void WSpinnyGLSL::drawTexture(QOpenGLTexture* texture) {
m_textureShader.setAttributeArray(
texcoordLocation, GL_FLOAT, texarray.data(), 2);

texture->bind();
pTexture->bind();

glDrawArrays(GL_TRIANGLE_STRIP, 0, 4);

texture->release();
pTexture->release();
}

void WSpinnyGLSL::drawVinylQuality() {
Expand Down
2 changes: 1 addition & 1 deletion src/widget/wspinnyglsl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class WSpinnyGLSL : public WSpinnyBase, private QOpenGLFunctions {
void initializeGL() override;
void paintGL() override;
void resizeGL(int w, int h) override;
void drawTexture(QOpenGLTexture* texture);
void drawTexture(QOpenGLTexture* pTexture);
void cleanupGL();
void updateTextures();

Expand Down
14 changes: 7 additions & 7 deletions src/widget/wvumeterglsl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,15 @@ void WVuMeterGLSL::cleanupGL() {
doneCurrent();
}

void WVuMeterGLSL::drawTexture(QOpenGLTexture* texture,
void WVuMeterGLSL::drawTexture(QOpenGLTexture* pTexture,
const QRectF& targetRect,
const QRectF& sourceRect) {
const float texx1 = static_cast<float>(sourceRect.x() / texture->width());
const float texy1 = static_cast<float>(sourceRect.y() / texture->height());
const float texx1 = static_cast<float>(sourceRect.x() / pTexture->width());
const float texy1 = static_cast<float>(sourceRect.y() / pTexture->height());
const float texx2 = static_cast<float>(
(sourceRect.x() + sourceRect.width()) / texture->width());
(sourceRect.x() + sourceRect.width()) / pTexture->width());
const float texy2 = static_cast<float>(
(sourceRect.y() + sourceRect.height()) / texture->height());
(sourceRect.y() + sourceRect.height()) / pTexture->height());

const float posx1 = static_cast<float>(targetRect.x());
const float posy1 = static_cast<float>(targetRect.y());
Expand All @@ -176,9 +176,9 @@ void WVuMeterGLSL::drawTexture(QOpenGLTexture* texture,
m_textureShader.setAttributeArray(
texcoordLocation, GL_FLOAT, texarray.data(), 2);

texture->bind();
pTexture->bind();

glDrawArrays(GL_TRIANGLE_STRIP, 0, 4);

texture->release();
pTexture->release();
}
Loading