OpenEXROutput: skip dwa compression for single channel small tile images #2147
Conversation
src/openexr.imageio/exroutput.cpp
Outdated
| @@ -679,6 +679,11 @@ OpenEXROutput::spec_to_header(ImageSpec& spec, int subimage, | |||
| // on deep files (but allow "none" as well) | |||
| if (spec.deep && comp != "none") | |||
| comp = "zips"; | |||
| // Currently dwaa/b compression does not work on single channel with tile < 16 | |||
| if (spec.nchannels == 1 && spec.tile_width < 16 && spec.tile_height < 16 && | |||
There was a problem hiding this comment.
Is it if both width and height are < 16? Or if either one is?
I don't know/remember, just asking.
|
LGTM. I'll await your answer to the one question I had, and for the CI tests to return successfully. Then I'll merge. Thanks for the fix. |
|
Fixes #1844 |
|
It didn't seem to work when I added the above comment, so I think you have to say it, Ramon: If you add a comment that says "Fixes #1844", then when I merge this, it will automatically close the other issue ticket. |
|
For instance 31x15 and 16x8 fail, but 31x16 does not The patch does not fix all the underlying failure conditions with dwa compression, it fixes the ones that affect maketx (8x8, 4x4, etc), so: Fixes #1844 |
|
I'm not sure we should knowingly still have a bunch of failure cases. Should we just consider dwa + 1-channel to be unsafe for anything other than power-of-2 square tile sizes >= 16? And make everything else automatically revert to zip, until we know there's a robust fix on the OpenEXR side? (Keeping in mind that this will still allow dwaa for all the maketx and other uses that we truly care about.) |
…all or non power of two tile dimensions
|
Sounds good, there were still quite a lot of failing combinations for non power of 2 tile sizes under 64x64, see new patch. |
|
LGTM, I will merge after the CI tests come back. |
…ges (AcademySoftwareFoundation#2147) We've found that this is unsafe, dwaa/dwab compression crashes inside OpenEXR for certain tile sizes (only for 1-channel images, it seems). Until there is a fix on the exr end, we'll just revert to zip compression for the problematic cases. Power-of-two tiles larger than 16 seem fine, and that's all maketx uses anyway.
|
Thanks! |
PR AcademySoftwareFoundation#2147, addressing issue AcademySoftwareFoundation#1844, is related to an apparent OpenEXR bug wherein single channel tiled images where the tile dimensions were not a power of 2 and >= 16 would not work properly with dwa compression. We work around the problem by automatically substituting zip compression in those cases. But it was implemented with a subtle bug, forgetting that tile_size == 0 for untiled images, and thus we were silently switching from dwa to zip compression for all *untiled* single channel images of any size (which presumably were never problematic). This patch fixes the test so we are not changing the compression for the untiled files. There's a long Signed-off-by: Larry Gritz <lg@larrygritz.com>
PR #2147, addressing issue #1844, is related to an apparent OpenEXR bug wherein single channel tiled images where the tile dimensions were not a power of 2 and >= 16 would not work properly with dwa compression. We work around the problem by automatically substituting zip compression in those cases. But it was implemented with a subtle bug, forgetting that tile_size == 0 for untiled images, and thus we were silently switching from dwa to zip compression for all *untiled* single channel images of any size (which presumably were never problematic). This patch fixes the test so we are not changing the compression for the untiled files. Signed-off-by: Larry Gritz <lg@larrygritz.com>
…demySoftwareFoundation#3884) PR AcademySoftwareFoundation#2147, addressing issue AcademySoftwareFoundation#1844, is related to an apparent OpenEXR bug wherein single channel tiled images where the tile dimensions were not a power of 2 and >= 16 would not work properly with dwa compression. We work around the problem by automatically substituting zip compression in those cases. But it was implemented with a subtle bug, forgetting that tile_size == 0 for untiled images, and thus we were silently switching from dwa to zip compression for all *untiled* single channel images of any size (which presumably were never problematic). This patch fixes the test so we are not changing the compression for the untiled files. Signed-off-by: Larry Gritz <lg@larrygritz.com>
dwa? compression fails when saving single channel files with small tile sizes (< 16)
Larry determined in #1844 that the underlying issue is in OpenEXR.
While that gets fixed we should skip dwa compression for the failure cases. This patch replaces dwa compression with the default zip for the failing cases.