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

Add explicit casts around the usage of zlib datatypes to avoid warnings on Windows #1259

Merged
merged 2 commits into from
May 15, 2022

Conversation

fpsunflower
Copy link
Contributor

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

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>
@fpsunflower
Copy link
Contributor Author

While doing this, I surveyed the state of zlib compatible libraries online to see if there would be a more robust solution to this:

  • miniz: A single-file zlib compatible rewrite. It offers a zlib compatible interface (with a different name to prevent clashes) but follows the convention of using unsigned long for sizes. I looked to see what the code actually did under the hood and it simply punts if the sizes provided are larger than 32-bits (source). This is potentially appealing because it would let us drop the dependency on zlib, but means we would either have to patch miniz to do the right thing on 64-bit buffers, or try to address it ourselves with some wrapper code).
  • zlib-ng: A modern fork of zlib which has removed legacy cruft and implemented a number of speedups using modern instruction sets. They provide an ABI compatible interface which follows the behavior of using unsigned long. However they also support a "native" interface that switches the data-types to size_t (as one would expect). (See their porting guide for more details). In this mode, the APIs do the right thing and process memory buffers larger than 4Gb by compressing them in chunks (source)
  • libdeflate: A different API for the same compression format. It has a sane API using size_t, but some other details of the API are different and incompatible. I'm not sure that it would be easy to switch to this one easily.

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.

@fpsunflower
Copy link
Contributor Author

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.

Copy link
Contributor

@meshula meshula left a 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.

@meshula
Copy link
Contributor

meshula commented May 10, 2022

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.

@fpsunflower
Copy link
Contributor Author

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 OpenEXR and OpenEXRCore I wasn't sure how to cleanly re-use the compression bits without also putting them into the public API of OpenEXRCore ...

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.

@meshula
Copy link
Contributor

meshula commented May 10, 2022

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.

Copy link
Member

@cary-ilm cary-ilm left a 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.

@cary-ilm
Copy link
Member

And let's continue the discussion of zlib-ng/deflate and benchmarking on a separate thread.

@cary-ilm cary-ilm merged commit 74645c5 into AcademySoftwareFoundation:main May 15, 2022
@fpsunflower
Copy link
Contributor Author

The far distinction is a remnant from super old MSDOS era compilers. I has no meaning for operating systems from this century :)

@meshula
Copy link
Contributor

meshula commented May 17, 2022

heh, far meant that a pointer had extra data to index beyond 16 bits. That's how we got to all that 640k memory, baby. It's super specific to x86 segmented memory, why does microsoft keep this around?!

cary-ilm pushed a commit to cary-ilm/openexr that referenced this pull request Mar 3, 2023
…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>
cary-ilm pushed a commit that referenced this pull request Mar 5, 2023
…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>
@cary-ilm cary-ilm added the v3.1.6 label Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants