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 incorrect index calculations in image conversions in ktx create #735

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

VaderDev
Copy link
Contributor

@VaderDev VaderDev commented Jul 10, 2023

Fix incorrect index calculations in image conversions in ktx create (#733, #736)

@MarkCallow
Copy link
Collaborator

@aqnuep please review.

@MarkCallow
Copy link
Collaborator

Why did the user not receive any notice of the crash?

And, less importantly, why did it produce an output file on macOS?

@aqnuep
Copy link
Collaborator

aqnuep commented Jul 12, 2023

We discussed this change with @VaderY. It was a trivial oversight where row stride was incorrectly derived from the height instead of the width.

I'm not sure about the different behaviors across platforms, but I'd expect this typo would mainly only cause data corruption for non-square textures due to the incorrect row stride.

Copy link
Collaborator

@aqnuep aqnuep left a comment

Choose a reason for hiding this comment

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

LGTM.

@aqnuep
Copy link
Collaborator

aqnuep commented Jul 12, 2023

FYI, @MarkCallow, @VaderY also added a bunch of odd sized textures to the CTS to have better coverage on this front.

We apologize for the typo, but it's been a huge set of functionalities to cover with implementation and tests, and obviously this slipped through the cracks.

@VaderDev
Copy link
Contributor Author

Why did the user not receive any notice of the crash?

Technically the exit code was an indicator (it was 0xC0000005 = Access Violation). But as it was a bug and not an error that we should ever handle, there was no reasonable way to produce an error message about it.
There are ways with C++ to intercept such crashes (breakpad, crashbed, signals, etc) and attempt to collect more information, write out coredumps and notify the user. But it is not a trivial undertaking to implement or adopt these in a cross platform manner.

And, less importantly, why did it produce an output file on macOS?

Given some bad luck these kind of undefined behaviors are sometimes hidden on certain platform. When I checked in windows builds every compiler yielded a proper crash. Its possible that the application owned the memory pages that was written to and there was nothing important.

@MarkCallow MarkCallow merged commit 682f456 into KhronosGroup:main Jul 13, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants