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

Remove usage of experimental/private APIs of zstd #17374

Closed
akien-mga opened this issue Mar 9, 2018 · 8 comments · Fixed by #28158
Closed

Remove usage of experimental/private APIs of zstd #17374

akien-mga opened this issue Mar 9, 2018 · 8 comments · Fixed by #28158
Assignees
Milestone

Comments

@akien-mga
Copy link
Member

akien-mga commented Mar 9, 2018

Godot version:
3.0.2, current master (b842369)

OS/device including version:
Any.

Issue description:
We currently use experimental APIs from zstd, which are as such not exposed publicly in stable zstd releases compiled as shared libraries (i.e. those we could link against on Linux).

I don't think we have any reason to use such APIs, it was likely done without paying much attention to their availability, or lack thereof, depending on build options.

Steps to reproduce:
Build against system libzstd with builtin_zstd=no. Tested against zstd 1.3.2 and 1.3.4.

@akien-mga akien-mga added this to the 3.1 milestone Mar 9, 2018
@akien-mga
Copy link
Member Author

Specifically:

$ scons p=x11 builtin_zstd=no
[ 95%] Compiling ==> core/io/compression.cpp
core/io/compression.cpp: In static member function 'static int Compression::compress(uint8_t*, const uint8_t*, int, Compression::Mode)':
core/io/compression.cpp:83:33: error: 'ZSTD_p_compressionLevel' was not declared in this scope
    ZSTD_CCtx_setParameter(cctx, ZSTD_p_compressionLevel, zstd_level);
                                 ^
core/io/compression.cpp:83:68: error: 'ZSTD_CCtx_setParameter' was not declared in this scope
    ZSTD_CCtx_setParameter(cctx, ZSTD_p_compressionLevel, zstd_level);
                                                                    ^
core/io/compression.cpp:85:34: error: 'ZSTD_p_enableLongDistanceMatching' was not declared in this scope
     ZSTD_CCtx_setParameter(cctx, ZSTD_p_enableLongDistanceMatching, 1);
                                  ^
core/io/compression.cpp:86:34: error: 'ZSTD_p_windowLog' was not declared in this scope
     ZSTD_CCtx_setParameter(cctx, ZSTD_p_windowLog, zstd_window_log_size);
                                  ^
core/io/compression.cpp: In static member function 'static int Compression::decompress(uint8_t*, int, const uint8_t*, int, Compression::Mode)':
core/io/compression.cpp:177:95: error: 'ZSTD_DCtx_setMaxWindowSize' was not declared in this scope
    if (zstd_long_distance_matching) ZSTD_DCtx_setMaxWindowSize(dctx, 1 << zstd_window_log_size);
                                                                                               ^
scons: *** [core/io/compression.x11.tools.64.o] Error 1
scons: building terminated because of errors.

@tagcup
Copy link
Contributor

tagcup commented Mar 14, 2018

The reasoning was: it's currently the only way to access the long distance matching

@akien-mga
Copy link
Member Author

Thanks for providing some context. Is that feature really significant for Godot, or can we do without it until zstd makes it stable? Linux distros don't want being forced to bundle libraries when they're already provided as shared libraries by the system.

@tagcup
Copy link
Contributor

tagcup commented Mar 14, 2018

It depends I guess. If you have large data, long range distance matching may help a lot. But this might be relevant for only a handful of people out there. You should be able to remove it by reverting the commit f3436a8

@tagcup
Copy link
Contributor

tagcup commented Mar 14, 2018

Long distance matching requires at least zstd 1.3.2 but as far as I understand it's here to stay

@akien-mga
Copy link
Member Author

Long distance matching requires at least zstd 1.3.2 but as far as I understand it's here to stay

The problem is not the version, we can easily ask distro packagers to use zstd 1.3.2 or later, but this API is not exposed when compiling zstd as a shared library due to its experimental nature. So as it stands our build requirements are zstd >= 1.3.2 and statically linked (i.e. use the bundled one).

It depends I guess. If you have large data, long range distance matching may help a lot. But this might be relevant for only a handful of people out there. You should be able to remove it by reverting the commit f3436a8

I guess we could keep the code behind #ifdef's and make it opt-in, so that people who may have a use case for it can enable it. Once the API is made stable upstream, we can remove the option and enable it for everyone.

@akien-mga
Copy link
Member Author

Note: As discussed in #24754, it's likely not worth spending time making this optional as those features are on their way to reaching stable status in zstd 1.4.0.

@akien-mga akien-mga modified the milestones: 3.2, 3.1 Jan 20, 2019
@akien-mga akien-mga self-assigned this Apr 18, 2019
@akien-mga akien-mga modified the milestones: 3.1, 3.2 Apr 18, 2019
@akien-mga
Copy link
Member Author

Reopening, as even with 1.4.0 there is still one experimental method that we use ZSTD_DCtx_setMaxWindowSize, so it's still not possible to unbundle zstd.

I think I'll just drop the use of these advanced features.

@akien-mga akien-mga reopened this Apr 18, 2019
akien-mga added a commit to akien-mga/godot that referenced this issue Apr 18, 2019
One step towards fixing godotengine#17374 as most experimental APIs we use are now
part of the stable 1.4.0.
akien-mga added a commit to akien-mga/godot that referenced this issue Apr 18, 2019
`ZSTD_DCtx_setMaxWindowSize` is still part of the experimental API
(thus unexposed in the shared library). Upstream examples seem to
use `ZSTD_d_windowLogSize` instead, so it's probably what we should
use too.

Fixes godotengine#17374.
Distro packagers can now unbundle Zstd.
akien-mga added a commit that referenced this issue Apr 20, 2019
One step towards fixing #17374 as most experimental APIs we use are now
part of the stable 1.4.0.

(cherry picked from commit 88cb9bd)
akien-mga added a commit that referenced this issue Apr 20, 2019
`ZSTD_DCtx_setMaxWindowSize` is still part of the experimental API
(thus unexposed in the shared library). Upstream examples seem to
use `ZSTD_d_windowLogSize` instead, so it's probably what we should
use too.

Fixes #17374.
Distro packagers can now unbundle Zstd.

(cherry picked from commit 2026587)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants