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

Feature write uint8 uint16 uint32 float32 typedarrays #366

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

daumann
Copy link

@daumann daumann commented Jun 13, 2023

No description provided.

@KevinDonnot
Copy link

I juste leaved an issue regarding 1-Bit files (UInt1). Would it be possible to add this type to your work?
All my bests!

Copy link
Contributor

@DanielJDufour DanielJDufour left a comment

Choose a reason for hiding this comment

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

Excellent work so far! My only main request is to consider defaulting to Float64, so we can support an untyped Array of values that is outside the 0-255 range.

export function isTypedUintArray(input) {
if (ArrayBuffer.isView(input)) {
const ctr = input.constructor;
if (ctr === Uint8Array || ctr === Uint16Array || ctr === Uint32Array || ctr === Uint8ClampedArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch also including the clamped version of Uint8!

width,
};

const newGeoTiffAsBinaryData = await writeArrayBuffer((originalValues), metadata);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have an extra parens wrapping originalValues


const samplesPerPixel = ifd[277];
let elementSize = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a comment describing why you set the default to 4?

Copy link
Contributor

Choose a reason for hiding this comment

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

could we consider defaulting elementSize to 8, so we can support all floating point numbers by default?


forEach(values, (value, i) => {
if (!TypedArray) {
data[numBytesInIfd + i] = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would only work if the Array of values is within the 0-255 range. Is there any way we could update this to support Float64 by default? Could we do view.setFloat64 to support any floating point number?

Open to your thoughts!

metadata.StripByteCounts = [numBands * height * width];

// default for Uint8
let elementSize = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we want to change the default to 8 for Float64 instead of Uint8? Ideally, the writer will work for the greatest number of use cases by default. If someone wants Uint8, they could pass in a Uint8Array of values instead of an Array.

@DanielJDufour
Copy link
Contributor

@daumann and @KevinDonnot , I would love to see 1-bit support soon, but I would recommend doing that in a follow up PR.

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.

3 participants