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 SignedByte and SignedShort enum variants #234

Merged
merged 6 commits into from
Jul 22, 2024

Conversation

weiji14
Copy link
Contributor

@weiji14 weiji14 commented May 17, 2024

Support SignedByte (i8) and SignedShort (i16) types by:

  • Adding tiff::decoder::ifd::Value::SignedByte and tiff::decoder::ifd::Value::SignedShort enum variants
  • Adding TiffFormatError::SignedByteExpected and TiffFormatError::SignedShortExpected enum variants
  • Adding two new into_i8 and into_i16 methods under impl Value in src/decoder/ifd.rs

Also included two new TIFF files to test roundtrip decoding/encoding:

  • tests/images/minisblack-1c-i8b.tiff
    • Created using gdal_translate -ot Int8 -co PIXELTYPE=SIGNEDBYTE -scale 0 255 \-128 128 tests/images/minisblack-1c-8b.tiff tests/images/minisblack-1c-i8b.tiff
  • tests/images/minisblack-1c-i16b.tiff
    • Created using gdal_translate -ot Int16 -co PIXELTYPE=SIGNEDBYTE -scale 0 65536 \-32767 32767 tests/images/minisblack-1c-16b.tiff tests/images/minisblack-1c-i16b.tiff

References:

Addresses #204 (comment)

Add SignedByte(i8) and SignedShort(i16) enum variants to tiff::decoder::ifd::Value.
Ensure that Value::SignedByte and Value::SignedShort to the into_i32, into_i64, into_i32_vec, into_i64_vec methods.
@weiji14 weiji14 marked this pull request as ready for review May 27, 2024 23:18
@fintelia
Copy link
Contributor

fintelia commented Jun 2, 2024

Shouldn't there be more logic to actually handle reading/writing these types?

Check that an SSHORT (int16) image can be read and written to properly. The minisblack-1c-i16b.tiff image was created using the command `gdal_translate -ot Int16 -co PIXELTYPE=SIGNEDBYTE -scale 0 65536 \-32767 32767 tests/images/minisblack-1c-16b.tiff tests/images/minisblack-1c-i16b.tiff`.
Check that an SBYTE (int8) image can be read and written to properly. The minisblack-1c-i8b.tiff image was created using the command `gdal_translate -ot Int8 -co PIXELTYPE=SIGNEDBYTE -scale 0 255 \-128 128 tests/images/minisblack-1c-8b.tiff tests/images/minisblack-1c-i8b.tiff`.
@weiji14 weiji14 changed the title Add SignedByte and SignedShort variants to Value enum Add SignedByte and SignedShort enum variants Jun 2, 2024
@weiji14
Copy link
Contributor Author

weiji14 commented Jun 3, 2024

Shouldn't there be more logic to actually handle reading/writing these types?

I've added a couple of roundtrip tests to check reading/writing SByte and SShort TIFFs, see if that looks ok.

Btw, I looked under https://github.com/libsdl-org/libtiff/tree/v4.6.0/test/images and couldn't find any obvious example TIFF files of int8/int16 types, so I just converted the uint8/uint16 tiffs using gdal_translate with some scaling. Happy to exchange those to better samples if needed.

weiji14 added a commit to georust/geotiff that referenced this pull request Jul 22, 2024
Use tiff::decoder::ifd::Value (https://docs.rs/tiff/0.9.1/tiff/decoder/ifd/enum.Value.html) instead of the TagValue enum in src/lowlevel.rs. Note that SignedByte and SignedShort variants are currently commented out, wait for image-rs/image-tiff#234. Also fixed some clippy warnings.
@weiji14
Copy link
Contributor Author

weiji14 commented Jul 22, 2024

Gentle ping @fintelia to see if this PR looks ok? I'm slowly refactoring the geotiff crate downstream to rely on image-tiff's enums, and it would be nice to have this PR wrapped up so that I can continue with georust/geotiff#15.

@fintelia fintelia merged commit 307cd8b into image-rs:master Jul 22, 2024
6 checks passed
@weiji14 weiji14 deleted the add-tiff-types branch July 22, 2024 03:41
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.

None yet

2 participants