-
Notifications
You must be signed in to change notification settings - Fork 644
oiiotool DWAA unexpected behavior #3883
-
Howdy all, I've been messing around making a batch texture conversion script with oiiotool. I want to resave a bunch of 16- and 32-bit PIZ Substance Painter exports as DWAA. In my PowerShell script this
oiiotool $_.FullName -compression DWAA -o $output
creates DWAA 45 EXRs for all the 16-bit files and ZIP for the 32-bit ones. Is this expected behavior? I suppose it's logical that you wouldn't want 32-bit files to be in a compressed format, but it seems odd that it just silently does this. Thanks!
Beta Was this translation helpful? Give feedback.
All reactions
I think the right thing to do is make the switch to zip happen only for tiled images, i.e. fix the comment and add a
&& spec.tile_width > 0
to the conditional so that it doesn't accidentally sweep up ALL single channel scanline files. Make sure it passes tests, and hope we're not making a mistake.
There is a small chance that if there are also problems with dwa for certain sized 1-channel scanline images after all, this will expose that bug that we had been avoiding. Does anybody want to speak for or against going for it?
Replies: 4 comments 5 replies
-
Can you try this sequence and show us the full output, please?
oiiotool -info -v my32bit.exr
oiiotool my32bit.exr -compression dwaa -o out.exr
oiiotool -info -v out.exr
Beta Was this translation helpful? Give feedback.
All reactions
-
I mean, obviously, substituting the actual name of one of your 32 bit input files where I have written "my32bit.exr".
Beta Was this translation helpful? Give feedback.
All reactions
-
Thanks for the quick reply @lgritz. Here's the output:
Reading C:\Users\Brian\Desktop\test\mat_head.disp.1001.exr
C:\Users\Brian\Desktop\test\mat_head.disp.1001.exr : 4096 x 4096, 1 channel, float openexr
channel list: Y
compression: "piz"
PixelAspectRatio: 1
screenWindowCenter: 0, 0
screenWindowWidth: 1
oiio:ColorSpace: "Linear"
oiio:subimages: 1
oiiotool C:\Users\Brian\Desktop\test\mat_head.disp.1001.exr -compression dwaa -o out.exr
Reading out.exr
out.exr : 4096 x 4096, 1 channel, float openexr
channel list: Y
compression: "zip"
DateTime: "2023:06:13 22:03:54"
PixelAspectRatio: 1
screenWindowCenter: 0, 0
screenWindowWidth: 1
Software: "OpenImageIO 2.4.1.1dev : oiiotool.exe C:/Users/Brian/Desktop/test/mat_head.disp.1001.exr -compression dwaa -o out.exr"
Exif:ImageHistory: "oiiotool.exe C:/Users/Brian/Desktop/test/mat_head.disp.1001.exr -compression dwaa -o out.exr"
oiio:ColorSpace: "Linear"
oiio:subimages: 1
Beta Was this translation helpful? Give feedback.
All reactions
-
Aha, I believe the problem is not that it's a 'float' image, but that it's one channel.
There is this suspicious code in OIIO:
// For single channel images, dwaa/b compression only seems to work reliably // when size > 16 and size is a power of two if (spec.nchannels == 1 && Strutil::istarts_with(comp, "dwa") && ((spec.tile_width < 16 && spec.tile_height < 16) || !ispow2(spec.tile_width) || !ispow2(spec.tile_height))) { comp = "zip"; }
I'm embarrassed to say that what the code does is not a match for what the comment says.
- The comment implies that dwa compression has problems for single channel images when the image dimensions are not powers of 2 and at least 16, and switches to zip in that case.
- The code switches away from dwa compression for single channel images when the tile sizes are not powers of 2 or are too small.
Except, oops, even if the code version is correct, this conditional as implemented doesn't account for the fact that non-tiled images will be marked as having tile size 0, which is < 16, and thus ALL scanline images will mysteriously disable dwaa compression. That sounds like a bug.
Is the comment correct, or is the code correct? My bet would be that the code is closer to correct, i.e., a bug in openexr's dwa compression is more likely to be hiding in the rare case of non-power-of-2 tiles, and not in the more common case of any image dimensions being non power of 2 (though that's by no means certain -- single channels are not the norm, so it's still possible such a bug might go undiagnosed).
And whichever one was right, is this still a bug that needs a workaround in modern OpenEXR? If not, was it fixed early enough in OpenEXR history that we no longer have to worry about the versions where it was problematic?
Beta Was this translation helpful? Give feedback.
All reactions
-
I think the right thing to do is make the switch to zip happen only for tiled images, i.e. fix the comment and add a
&& spec.tile_width > 0
to the conditional so that it doesn't accidentally sweep up ALL single channel scanline files. Make sure it passes tests, and hope we're not making a mistake.
There is a small chance that if there are also problems with dwa for certain sized 1-channel scanline images after all, this will expose that bug that we had been avoiding. Does anybody want to speak for or against going for it?
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1
-
The weird thing is that when I look at those lines, I have a strong sense of deja vu, as if I have fairly recently noticed this very discrepancy between the comment and code, and in my memory, I already fixed it. Is this just a memory glitch on my part? Or did I fix it in a branch and somehow forget to ever submit the PR?
Beta Was this translation helpful? Give feedback.
All reactions
-
Proposed fix: #3884
Beta Was this translation helpful? Give feedback.
All reactions
-
@lgritz Do you expect this fix to go live soon? I can build with vcpkg, wondering if I should attempt to patch the file myself in the meantime. Thank you!
Beta Was this translation helpful? Give feedback.
All reactions
-
Usually, I do a patch release on the first day of each month (though I'm willing to do it at other times for critical bugs), and VcPkg tends to pick it up and update their distribution within a few weeks. If you can't wait that long, then I would suggest applying the patch (it's pretty small) on your end and building from source (on top of the current "release" branch).
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1