Skip to content

feat: add zstd via fzstd#263

Merged
kylebarron merged 18 commits intomainfrom
zstd
Feb 28, 2026
Merged

feat: add zstd via fzstd#263
kylebarron merged 18 commits intomainfrom
zstd

Conversation

@gadomski
Copy link
Contributor

No description provided.

@gadomski gadomski requested a review from kylebarron February 25, 2026 13:12
@gadomski gadomski self-assigned this Feb 25, 2026
@github-actions github-actions bot added the feat label Feb 25, 2026
Comment on lines -44 to -47
const tile = await self.image.getTile(x, y, options);
if (tile === null) {
throw new Error("Tile not found");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might still want to have a relevant check for whether the tile/band being fetched is in the range of the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we get this from the check below?

        const result = await self.image.getBytes(offset, imageSize, {
          signal: opts.signal,
        });
        if (result === null) {
          throw new Error(
            `Band ${b} tile not found at index ${tileIndex}`,
          );
        }

Sorry for the question w/o the answer, I'm still building my mental model here.

@kylebarron
Copy link
Member

As a general comment, I think this PR sounds good to set the default decompressor to fzstd, since it says it's only 8KB as a dependency. And then we should document and allow end users to override with a WASM-based zstd dependency if they prefer.

@gadomski
Copy link
Contributor Author

As a general comment, I think this PR sounds good to set the default decompressor to fzstd, since it says it's only 8KB as a dependency. And then we should document and allow end users to override with a WASM-based zstd dependency if they prefer.

Something like 653cf42?

@kylebarron
Copy link
Member

kylebarron commented Feb 27, 2026

As a general comment, I think this PR sounds good to set the default decompressor to fzstd, since it says it's only 8KB as a dependency. And then we should document and allow end users to override with a WASM-based zstd dependency if they prefer.

Something like 653cf42?

No I think that's extra unnecessary complexity..

We can export the decoder registry from global scope. And then the user can just do

registry.set(Compression.Zstd, () =>
  import("user-defined zstd module").then((m) => m.decode),
);

Or, if they want to always import the module, they can do

import { decode } from "some-zstd-decoder";

registry.set(Compression.Zstd, () => Promise.resolve(decode));

I think it's probably safe to assume that users won't want different decoders for the same compression type for different images. I.e. it's fine for decoders to be a single registry in global scope.

@kylebarron
Copy link
Member

@gadomski can you split this PR into two? One for adding ZSTD and a separate PR for loading band-separate data?

@gadomski
Copy link
Contributor Author

can you split this PR into two? One for adding ZSTD and a separate PR for loading band-separate data?

Done: #276


[LERC]: https://github.com/Esri/lerc

You can also override the built-in decoders with your own by using `registry`. For example, to use a custom zstd decoder:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@kylebarron kylebarron merged commit 092861f into main Feb 28, 2026
3 checks passed
@kylebarron kylebarron deleted the zstd branch February 28, 2026 16:33
@ds-release-bot ds-release-bot bot mentioned this pull request Feb 28, 2026
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.

2 participants