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

preroll waveformrenderer using rendergraph #14190

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Jan 18, 2025

Ports the preroll waveformrenderer to a rendergraph node. This can be reviewed and merged independently from the PRs for the other waveformrenderers.

Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

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

LGTM! Just a nit with QStringLiteral

src/waveform/renderers/waveformwidgetrenderer.cpp Outdated Show resolved Hide resolved
src/waveform/renderers/waveformwidgetrenderer.cpp Outdated Show resolved Hide resolved
src/waveform/renderers/waveformwidgetrenderer.cpp Outdated Show resolved Hide resolved
m0dB and others added 4 commits January 21, 2025 00:44
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

couple nits. Love seeing a PR with a negative line diff ;)

Comment on lines -23 to +24
QImage image(imagePixelW, imagePixelH, QImage::Format_ARGB32_Premultiplied);
QImage image(imagePixelW, imagePixelH, QImage::Format_RGBA8888_Premultiplied);
Copy link
Member

Choose a reason for hiding this comment

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

can you quickly explain your rationale behind this?
According to the QImage docs:

Note: Avoid most rendering directly to most of these formats using QPainter. Rendering is best optimized to the Format_RGB32 and Format_ARGB32_Premultiplied formats, and secondarily for rendering to the Format_RGB16, Format_RGBX8888, Format_RGBA8888_Premultiplied, Format_RGBX64 and Format_RGBA64_Premultiplied formats

Comment on lines +154 to +156
dynamic_cast<PatternMaterial&>(material())
.setTexture(std::make_unique<Texture>(m_waveformRenderer->getContext(),
drawPrerollImage(m_markerLength,
Copy link
Member

Choose a reason for hiding this comment

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

does this allocate continuously in the hot paintGL path?

Comment on lines +126 to +127
const int reserved = (preRollVisible ? numVerticesPerRectangle : 0) +
(postRollVisible ? numVerticesPerRectangle : 0);
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to move closer to the first use? and potentially give it a better name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants