-
Notifications
You must be signed in to change notification settings - Fork 617
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
Add explicit casts around the usage of zlib datatypes to avoid warnings on Windows #1259
Add explicit casts around the usage of zlib datatypes to avoid warnings on Windows #1259
Conversation
Signed-off-by: Chris Kulla <ckulla@gmail.com>
…gs on Windows Windows defines 'long' to be 32-bits, even for 64-bit builds. This causes MSVC to complain about most of the code surrounding calls to the zlib API. Luckily OpenEXR uses a minimal set of calls from zlib: * compress2 * compressBound * uncompress This change focuses on preserving the current behavior, but adds explicit casts to the appropriate types. Future attemps to address this should grep for uLong and the three functions above. As part of this cleanup, I also found a few places that had bespoke implementations of compressBound which I replaced with a call the real function. Cleaned up a stray use of halfLimits.h which is a deprecated header and is not required. Signed-off-by: Chris Kulla <ckulla@gmail.com>
While doing this, I surveyed the state of zlib compatible libraries online to see if there would be a more robust solution to this:
I suspect that zlib-ng is currently the fastest choice. The "native" API would solve the 64-bit issue for windows (probably only meaningful for extremely large (deep) images). Whatever we choose, any change in dependencies would have to be timed with a major release at least. |
Actually I forgot about this post which seems to have successfully swapped in libdeflate. So I guess it is a valid contender as well. Optimizing the creation of compressors may take some more thought though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clean up looks good to me.
I wonder if a good next step might be to create a "compression-test" fork, and perhaps put in an abstract compression interface so that the different libraries can be easily tested and benchmarked. It seems to me like it would be valuable to be able to slot in different comparisons for testing, as @aras-p did in his blog series. We could then have a go at expanding the platform coverage and comparisons to complement his work. |
Yup that sounds like a good next step. I was thinking of doing that from a code re-use perspective, but with the code now split between Since posting this patch I noticed that zlib also recently did a release including some speedups ... So having a way to measure seems like the safest way to proceed, we just need to agree on a benchmark methodology. I'm not sure I have access to enough EXR data at the moment to volunteer for this though. |
the reason i was thinking of a branch, is that architectural work to abstract the compression is one bit of work. stubbing in a bunch of compressors is another, and working out and implementing a benchmarking scheme is another. It could be a longer term effort by interested parties, rather than one person needing to do everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks straightforward enough to me, with the caveat that I know absolutely nothing about the MSC/MSDOC "far" type keyword, but from the comments in zconf.h, replacing uLongf with uLong should be fine.
And let's continue the discussion of zlib-ng/deflate and benchmarking on a separate thread. |
The |
heh, |
…gs on Windows (AcademySoftwareFoundation#1259) * Add vscode to .gitignore Signed-off-by: Chris Kulla <ckulla@gmail.com> * Add explicit casts around the usage of zlib datatypes to avoid warnings on Windows Windows defines 'long' to be 32-bits, even for 64-bit builds. This causes MSVC to complain about most of the code surrounding calls to the zlib API. Luckily OpenEXR uses a minimal set of calls from zlib: * compress2 * compressBound * uncompress This change focuses on preserving the current behavior, but adds explicit casts to the appropriate types. Future attemps to address this should grep for uLong and the three functions above. As part of this cleanup, I also found a few places that had bespoke implementations of compressBound which I replaced with a call the real function. Cleaned up a stray use of halfLimits.h which is a deprecated header and is not required. Signed-off-by: Chris Kulla <ckulla@gmail.com>
…gs on Windows (#1259) * Add vscode to .gitignore Signed-off-by: Chris Kulla <ckulla@gmail.com> * Add explicit casts around the usage of zlib datatypes to avoid warnings on Windows Windows defines 'long' to be 32-bits, even for 64-bit builds. This causes MSVC to complain about most of the code surrounding calls to the zlib API. Luckily OpenEXR uses a minimal set of calls from zlib: * compress2 * compressBound * uncompress This change focuses on preserving the current behavior, but adds explicit casts to the appropriate types. Future attemps to address this should grep for uLong and the three functions above. As part of this cleanup, I also found a few places that had bespoke implementations of compressBound which I replaced with a call the real function. Cleaned up a stray use of halfLimits.h which is a deprecated header and is not required. Signed-off-by: Chris Kulla <ckulla@gmail.com>
Windows defines 'long' to be 32-bits, even for 64-bit builds. This causes MSVC to complain about most of the code surrounding calls to the zlib API.
Luckily OpenEXR uses a minimal set of calls from zlib:
This change focuses on preserving the current behavior, but adds explicit casts to the appropriate types. Future attemps to address this should grep for uLong and the three functions above.
As part of this cleanup, I also found a few places that had bespoke implementations of compressBound which I replaced with a call the real function.
Cleaned up a stray use of halfLimits.h which is a deprecated header and is not required.