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

Fix null pointer access and avoid nested allocations with QOpenGLTexture #13043

Merged
merged 5 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1485,7 +1485,7 @@ else()
src/shaders/textureshader.cpp
src/shaders/unicolorshader.cpp
src/shaders/vinylqualityshader.cpp
src/util/texture.cpp
src/util/opengltexture2d.cpp
src/waveform/renderers/allshader/matrixforwidgetgeometry.cpp
src/waveform/renderers/allshader/waveformrenderbackground.cpp
src/waveform/renderers/allshader/waveformrenderbeat.cpp
Expand Down
33 changes: 33 additions & 0 deletions src/util/opengltexture2d.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#include "util/opengltexture2d.h"

#include <QPixmap>

#include "widget/paintable.h"

OpenGLTexture2D::OpenGLTexture2D()
: QOpenGLTexture(QOpenGLTexture::Target2D){};

void OpenGLTexture2D::setData(const QImage& image) {
destroy();
if (!image.isNull()) {
QOpenGLTexture::setData(image);
setMinMagFilters(QOpenGLTexture::Linear, QOpenGLTexture::Linear);
setWrapMode(QOpenGLTexture::ClampToEdge);
}
};

void OpenGLTexture2D::setData(const QPixmap& pixmap) {
setData(pixmap.toImage());
};

void OpenGLTexture2D::setData(const QSharedPointer<Paintable>& pPaintable) {
if (pPaintable) {
setData(pPaintable->toImage());
}
};

void OpenGLTexture2D::setData(const std::shared_ptr<QImage>& pImage) {
if (pImage) {
setData(*pImage);
}
};
16 changes: 16 additions & 0 deletions src/util/opengltexture2d.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#pragma once

#include <QOpenGLTexture>
#include <QSharedPointer>

class Paintable;

Copy link
Contributor

@m0dB m0dB Apr 4, 2024

Choose a reason for hiding this comment

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

maybe add a comment that explains that this is an extension of QOpenGLTexture, with additional methods to set the texture data, and default settings for filter and wrap mode.

class OpenGLTexture2D : public QOpenGLTexture {
public:
OpenGLTexture2D();

void setData(const QImage& image);
void setData(const QPixmap& pixmap);
void setData(const QSharedPointer<Paintable>& pPaintable);
void setData(const std::shared_ptr<QImage>& pImage);
};
37 changes: 0 additions & 37 deletions src/util/texture.cpp

This file was deleted.

12 changes: 0 additions & 12 deletions src/util/texture.h

This file was deleted.

15 changes: 8 additions & 7 deletions src/waveform/renderers/allshader/waveformrendererpreroll.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
#include <array>

#include "skin/legacy/skincontext.h"
#include "util/texture.h"
#include "waveform/renderers/allshader/matrixforwidgetgeometry.h"
#include "waveform/renderers/waveformwidgetrenderer.h"
#include "widget/wskincolor.h"

namespace {
std::unique_ptr<QOpenGLTexture> generateTexture(float markerLength,
void generateTexture(OpenGLTexture2D* pTexture,
Copy link
Member

Choose a reason for hiding this comment

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

I really don't see how the tradeoff in code quality makes sense. There is no reason to introduce an out-parameter here IMO. If the extra allocation bothers you, why not return it by value directly?

Copy link
Member Author

@daschuer daschuer Apr 5, 2024

Choose a reason for hiding this comment

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

This is this no out parameter which may be considered as bad code quality these days. It just updates the texture via pointer because OpenGLTexture2D has no copy constructor . I kept just to avoid moving code around. I will now make it a normal member function as usual and rename it to avoid future confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored to become drawPrerollImage() returning the temporary QImage.

float markerLength,
float markerBreadth,
float devicePixelRatio,
QColor color) {
Expand Down Expand Up @@ -54,7 +54,7 @@ std::unique_ptr<QOpenGLTexture> generateTexture(float markerLength,
painter.drawPath(path);
painter.end();

return createTexture(image);
pTexture->setData(image);
}
} // anonymous namespace

Expand Down Expand Up @@ -120,13 +120,14 @@ void WaveformRendererPreroll::paintGL() {
// has changed size last time.
m_markerLength = markerLength;
m_markerBreadth = markerBreadth;
m_pTexture = generateTexture(m_markerLength,
generateTexture(&m_texture,
m_markerLength,
m_markerBreadth,
m_waveformRenderer->getDevicePixelRatio(),
m_color);
}

if (!m_pTexture) {
if (!m_texture.isStorageAllocated()) {
return;
}

Expand All @@ -146,7 +147,7 @@ void WaveformRendererPreroll::paintGL() {
m_shader.setUniformValue(matrixLocation, matrix);
m_shader.setUniformValue(textureLocation, 0);

m_pTexture->bind();
m_texture.bind();

const float end = m_waveformRenderer->getLength();

Expand Down Expand Up @@ -191,7 +192,7 @@ void WaveformRendererPreroll::paintGL() {
(end - x) / markerLength);
}

m_pTexture->release();
m_texture.release();

m_shader.disableAttributeArray(positionLocation);
m_shader.disableAttributeArray(texcoordLocation);
Expand Down
3 changes: 2 additions & 1 deletion src/waveform/renderers/allshader/waveformrendererpreroll.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "shaders/patternshader.h"
#include "util/class.h"
#include "util/opengltexture2d.h"
#include "waveform/renderers/allshader/vertexdata.h"
#include "waveform/renderers/allshader/waveformrenderer.h"

Expand Down Expand Up @@ -33,7 +34,7 @@ class allshader::WaveformRendererPreroll final : public allshader::WaveformRende
QColor m_color;
float m_markerBreadth{};
float m_markerLength{};
std::unique_ptr<QOpenGLTexture> m_pTexture;
OpenGLTexture2D m_texture;

DISALLOW_COPY_AND_ASSIGN(WaveformRendererPreroll);
};
30 changes: 16 additions & 14 deletions src/waveform/renderers/allshader/waveformrendermark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include <QPainterPath>

#include "util/colorcomponents.h"
#include "util/texture.h"
#include "waveform/renderers/allshader/matrixforwidgetgeometry.h"
#include "waveform/renderers/allshader/rgbadata.h"
#include "waveform/renderers/allshader/vertexdata.h"
Expand All @@ -20,15 +19,15 @@
// then used as textures to be drawn with a GLSL shader.

class TextureGraphics : public WaveformMark::Graphics {
std::unique_ptr<QOpenGLTexture> m_pTexture;

public:
TextureGraphics(std::unique_ptr<QOpenGLTexture>&& pTexture)
: m_pTexture{std::move(pTexture)} {
}
QOpenGLTexture* texture() const {
return m_pTexture.get();
TextureGraphics() = default;

QOpenGLTexture* texture() {
return &m_texture;
}

private:
OpenGLTexture2D m_texture;
};

// Both allshader::WaveformRenderMark and the non-GL ::WaveformRenderMark derive
Expand Down Expand Up @@ -225,11 +224,12 @@ void allshader::WaveformRenderMark::paintGL() {
const float currentMarkPoint = std::floor(
static_cast<float>(m_waveformRenderer->getPlayMarkerPosition() *
m_waveformRenderer->getLength()));
if (m_pPlayPosMarkTexture) {
const float markHalfWidth = m_pPlayPosMarkTexture->width() / devicePixelRatio / 2.f;

if (m_playPosMarkTexture.isStorageAllocated()) {
const float markHalfWidth = m_playPosMarkTexture.width() / devicePixelRatio / 2.f;
const float drawOffset = currentMarkPoint - markHalfWidth;

drawTexture(drawOffset, 0.f, m_pPlayPosMarkTexture.get());
drawTexture(drawOffset, 0.f, &m_playPosMarkTexture);
}
}

Expand Down Expand Up @@ -297,7 +297,7 @@ void allshader::WaveformRenderMark::updatePlayPosMarkTexture() {
}
painter.end();

m_pPlayPosMarkTexture = createTexture(image);
m_playPosMarkTexture.setData(image);
}

void allshader::WaveformRenderMark::drawTriangle(QPainter* painter,
Expand All @@ -321,6 +321,8 @@ void allshader::WaveformRenderMark::resizeGL(int, int) {
}

void allshader::WaveformRenderMark::updateMarkImage(WaveformMarkPointer pMark) {
pMark->m_pGraphics = std::make_unique<TextureGraphics>(
createTexture(pMark->generateImage(m_waveformRenderer->getDevicePixelRatio())));
auto pTextureGraphics = std::make_unique<TextureGraphics>();
pTextureGraphics->texture()->setData(
pMark->generateImage(m_waveformRenderer->getDevicePixelRatio()));
pMark->m_pGraphics = std::move(pTextureGraphics);
}
9 changes: 5 additions & 4 deletions src/waveform/renderers/allshader/waveformrendermark.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "shaders/rgbashader.h"
#include "shaders/textureshader.h"
#include "util/opengltexture2d.h"
#include "waveform/renderers/allshader/waveformrendererabstract.h"
#include "waveform/renderers/waveformrendermarkbase.h"

Expand Down Expand Up @@ -44,12 +45,12 @@ class allshader::WaveformRenderMark : public ::WaveformRenderMarkBase,
QPointF p2,
QPointF p3);

mixxx::RGBAShader m_rgbaShader;
mixxx::TextureShader m_textureShader;
std::unique_ptr<QOpenGLTexture> m_pPlayPosMarkTexture;

void drawMark(const QRectF& rect, QColor color);
void drawTexture(float x, float y, QOpenGLTexture* texture);

mixxx::RGBAShader m_rgbaShader;
mixxx::TextureShader m_textureShader;
OpenGLTexture2D m_playPosMarkTexture;

DISALLOW_COPY_AND_ASSIGN(WaveformRenderMark);
};
Loading
Loading