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

Added LERC_ZSTD decompression #397

Merged
merged 4 commits into from
Oct 27, 2023
Merged

Conversation

TheMrCam
Copy link
Contributor

I noticed that Planetary Computer COGs are compressed with LERC_ZSTD and could not be read by geotiff.js.

I fumbled around a bit to figure out a working implementation of the Zstandard decoder since there's a couple Javascript ports, so I figured I'd make this PR in case it can be improved or merged.

The zstddec library utilizes WebAssembly (which I'm rather unexperienced with) and getting it properly initialized was the main pain point; I'm sure it could be cleaner but this appears to work for me.

The "DOM" types in tsconfig appear to be necessary (based on microsoft/TypeScript-DOM-lib-generator#826) but there might be better options, I just did the quickest fix to get it building.

I didn't notice until after I started on this that addDecoder could override current decoder implementations, but I'm struggling with getting that working in my own project and including this in an official release would make my job a smidge easier.

@constantinius
Copy link
Member

Hey @TheMrCam

Thanks for supplying this integration! This is indeed very cool. Unfortunately, since the underlying (de-)compressor uses WASM, it seems to break testing.

Do you think there is a way around this?

@TheMrCam
Copy link
Contributor Author

Unfortunately, since the underlying (de-)compressor uses WASM, it seems to break testing.

Do you think there is a way around this?

Hello @constantinius, thanks for taking a look at this! WASM is definitely not my strong suit, but I assume there should be a way around it for testing since the package seems to work when integrated into my project. Also, the failed checks in CI all pass on my local machine. I'll dig around a bit to see what I can do.

@constantinius
Copy link
Member

@TheMrCam
I see that node has added WASM support (not sure in which version). We are currently using node v14 in testing, which seems ancient, and has reached end end-of-life this April.

I'll try an update to Node v20, which is the current LTS version, to see if the tests pass then.

@constantinius
Copy link
Member

@TheMrCam
My naive update of Node did not seem to have worked, there seems to be a regression now: https://github.com/geotiffjs/geotiff.js/actions/runs/6558959613/job/17813580079?pr=398

Which node version are you using? How did you test locally?

@TheMrCam
Copy link
Contributor Author

@constantinius The Node update results actually looks as I expected - I'm getting the same 6 test failures locally (on Node v18.12.1). I wasn't sure if they were actual errors or if I wasn't setting things up properly, but now that I know they come from upgrading from Node 14.x I should be able to figure out what else needs to change.

It looks like there's 3 separate issues that cause these tests to fail:

  • Uncaught TypeError [ERR_INVALID_URL_SCHEME]: The URL must be of scheme file - related to web-worker
  • Error: done() called multiple times - related to mocha
  • TypeError: Cannot set properties of undefined (setting 'idle') - related to src/pool.js:77

I believe these should all be fixable and am looking into it now, and I will update if my assessment changes.

@TheMrCam
Copy link
Contributor Author

@constantinius I believe I found the solution, or at least it now tests OK locally & still works great integrated into my package. I did also update the ci.yml Node version to get the checks working, which absorbs your PR #398; if that's unnecessary or undesired, I can roll it back.

@constantinius constantinius merged commit 5421ef1 into geotiffjs:master Oct 27, 2023
1 check passed
@constantinius
Copy link
Member

Thanks @TheMrCam! This looks very good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants