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 crash when hiding waveforms in Deere #14170

Merged
merged 6 commits into from
Jan 14, 2025
Merged

Fix crash when hiding waveforms in Deere #14170

merged 6 commits into from
Jan 14, 2025

Conversation

daschuer
Copy link
Member

This fixes #14157

@@ -335,7 +335,7 @@ class MarkerGeometry {
};

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.

Comment on lines 391 to 392
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.

Comment on lines 404 to 406
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.


// 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.

@ronso0
Copy link
Member

ronso0 commented Jan 14, 2025

FWIW since this happens only with Deere (which is the only skin that uses waveform Singletons), I assume this is an issue with WSplitter/QSplitter and Singleton, like when the waveform part of the splitter is collapsed the waveform is not hidden or something?

@daschuer
Copy link
Member Author

like when the waveform part of the splitter is collapsed the waveform is not hidden or something?

Yes, the widget is sized to 1 before it is hidden due to the border the images have 0 height, which has caused the crash.

@daschuer
Copy link
Member Author

Done

@ronso0
Copy link
Member

ronso0 commented Jan 14, 2025

Ah, now I could reproduce the crash: you need to have one or more hotcue markers visible in waveform.

Can confirm the fix 👍

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this fix!

@ronso0 ronso0 merged commit cc92ef1 into mixxxdj:2.5 Jan 14, 2025
13 checks passed
@ronso0 ronso0 changed the title Fix crash when hiding waveforms Fix crash when hiding waveforms in Deere Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants