PNG: Fix check for too few axes, memory, orientations #2713
Conversation
|
While I believe the fix in de6d0c3 to be correct, couple of other things came up when running the relevant code in debugging mode.
|
- Fix failure to return dynamically allocated memory (two separate instances). - Fix orientation of exported PNGs in some use cases. - Fix excessive memory allocation for images spread across multiple PNG files. - When reading from multiple input PNG images, allocate memory for read buffers on a per-slice basis.
- Do not attempt to support writing bitwise images, even if the number of pixels in the width direction is a factor of 8. - Fix intensity scaling of bitwise images such that values of 0 and 255 are used in the output uint8 image. Add test data and tests for verifying operation of PNG export.
Lestropie
force-pushed
the
png_axes_fix
branch
2 times, most recently
from
0a275cd
to
386db1f
Compare
Partial regression of 76d7007.
Lestropie
changed the title
|
I ended up expanding the scope of this PR, as I discovered multiple other issues in the process of diagnosing and rectifying the original observation. This PR includes adding new test data, which demonstrates the capability to generate PNG images in different slice directions with the correct orientation (proving rectification of #2179), and new |
There was a problem hiding this comment.
Seems all good. Hit a slight snag when writing to a non-existent folder (segfault when I was expecting a more informative error message), but that last commit seems to sort that out. Otherwise, things seem to work with the very limited testing I've done on my system. But your battery of tests seems way more comprehensive, so I expect that's plenty enough validation to merge. 👍
|
I am unable to merge this PR:
I cannot re-review it myself, even after removing myself as assignee. So presumably it is checking for first commit / most commits. While I have the ability to use admin rights to bypass branch protections, we should maybe think about conforming to what's being semi-enforced as best practise. |
Lestropie
changed the title
- Fix erroneous modification of strides upon header concatenation; this replicates the fix included in 41c02e9 as part of #2806, but is necessary for the suite of fixes here to work. Note that this affects mrcat in addition to multi-image format handling. - Fix setting of strides upon PNG read. - Expand testing of PNG handling. Evaluation of read and write functionality is performed separately, with the reference data fully visible and verifiable within the test data repository. Some previous tests bundled read and write testing together, which obscured the erroneous intermediate interpretation.
|
I really hope this finally addresses all issues now. You can look at files
|
|
Seeking review on this as I want to subsequently merge back to |
- Omit explicit deletion of ImageIO::Base data where unnecessary; this can be handled by ImageIO::Base::close(). - FIx additional memory leaks in ImageIO::PNG.
|
No leaks across all |
Fixes erroneous check put in place in #2535 (3acf6cc).
@jdtournier #2535 looks wrong just from the code. Look at the check vs. the error message. This I expect to still catch the erroneous use case before segfaulting, but no longer block normal operation as seems to be happening with 3.0.4. Would ideally have some unit tests...