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

Fixed writing multiple StripOffsets to TIFF #8317

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Aug 19, 2024

The StripOffsets values don't really mean anything without associated image data, so I just did the same thing as when this tag only has one value and added the IFD offset to the values.

StripOffsets is overwritten with correct values when writing TIFF images, so this only matters when writing a non-TIFF image, where StripOffsets doesn't mean anything. The main benefit of this change is that it won't raise an exception now when saving a non-TIFF image with EXIF data copied from a TIFF image that has more than one strip.

Tests/test_file_tiff.py Outdated Show resolved Hide resolved
@Yay295 Yay295 force-pushed the tiff_exif_multistrip branch from c9d1abc to 9fb34d8 Compare August 19, 2024 13:16
@Yay295 Yay295 force-pushed the tiff_exif_multistrip branch from 9fb34d8 to 9707f88 Compare August 28, 2024 13:31
@Yay295 Yay295 force-pushed the tiff_exif_multistrip branch from 9707f88 to 362ffaf Compare September 18, 2024 19:50
raise NotImplementedError(msg)
value = self._pack("L", self._unpack("L", value)[0] + offset)
size, handler = self._load_dispatch[typ]
values = [val + offset for val in handler(self, data, self.legacy_api)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
values = [val + offset for val in handler(self, data, self.legacy_api)]
values = [val + offset for val in handler(self, data)]

Throwing this suggestion out there, see what happens - considering that this should be SHORT or LONG (or LONG8), it doesn't matter whether the legacy API is in use or not, as they're all passed to

def basic_handler(
self: ImageFileDirectory_v2, data: bytes, legacy_api: bool = True
) -> tuple[Any, ...]:
return self._unpack(f"{len(data) // size}{fmt}", data)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's more correct to pass the value since we have it. The value not being used is an implementation detail that this call shouldn't be concerned with.

@radarhere
Copy link
Member

StripOffsets is overwritten with correct values when writing TIFF images

If libtiff is used when saving, yes. Otherwise, no.

@radarhere radarhere changed the title Implement TIFF multistrip writing Fixed writing multiple StripOffsets to TIFF Sep 20, 2024
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@radarhere radarhere merged commit 3f24276 into python-pillow:main Sep 25, 2024
47 of 48 checks passed
@Yay295 Yay295 deleted the tiff_exif_multistrip branch September 25, 2024 12:40
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