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 support for reading TIFFs with PlanarConfiguration=2 #4641

Closed
wants to merge 1 commit into from

Conversation

kkopachev
Copy link
Contributor

Fixes #4621 .

Adding support for reading of compressed TIFFs with planar config = separate.

Since TIFF reading is done in tiles or strips (might be multiple rows), I've added reading it by sample and shuffling into image data. For 16bit images I had to create new single channel unpackers.

There is also special handling for RGBa images with separate plane storage, since unpacking it sample by sample would leave data in RGBa, so there is extra step required to convert it to RGBA.

I also thought of using line-interleaved unpackers, but they seem to be no use since tiles smaller than width of image.

@radarhere radarhere added the TIFF label May 23, 2020
@kkopachev
Copy link
Contributor Author

Thanks @radarhere for lint fixes.

Not sure what's going on with MacOS builds and why they are failing. I don't have Mac in possession to test it first hand, so I tried enabling TRACE logs for TiffDecode and pushed them into separate branch under my form to see in GA where it crashes. Surprisingly it finished with no errors. So I don't have any ideas where to look.
Any help appreciated.

@radarhere
Copy link
Member

I've created PR kkopachev#2 to resolve the macOS exception

@kkopachev
Copy link
Contributor Author

@radarhere Thank you for update, looks cleaner. I was trying to use higher-level language approach to swap pointer :)
I'm curious why original solution did not work. Any insights why it was failing?

@radarhere
Copy link
Member

No insights. If it was a simple mistake, then you would think it would be failing for more platforms than just macOS. I just found that ((*unpacker)[plane]) was where the error was being triggered, so I started trying alternatives and somewhere in there thought to remove the need for dereferencing entirely. So some of it was just trial and error using my macOS machine, some of it was using trial and error on GitHub Actions.

src/libImaging/Unpack.c Outdated Show resolved Hide resolved
src/libImaging/Unpack.c Outdated Show resolved Hide resolved
@wiredfool
Copy link
Member

I want to review this before merge for safety. Not that I’ll catch everything but I want to make a pass on it.

@kkopachev
Copy link
Contributor Author

I've added tests for new unpackers. Travis failure is unrelated.

@kkopachev
Copy link
Contributor Author

Any updates here? Would love this to make it into release.

@hugovk
Copy link
Member

hugovk commented Jun 29, 2020

@wiredfool Do you think you'll have time to check this before release day?

@wiredfool
Copy link
Member

Apparently not. It was on my list for last weekend, but I didn’t get to it.

@imfantuan
Copy link

imfantuan commented Dec 22, 2020

Hey guys, any updates here?
@hugovk @kkopachev @wiredfool

@aclark4life
Copy link
Member

@imfantuan Needs a rebase at least

@kkopachev kkopachev force-pushed the kk-planar-tiffs branch 5 times, most recently from b462924 to 3569b2f Compare December 31, 2020 06:12
@kkopachev
Copy link
Contributor Author

Rebased.
I had to skip added tests for big endian system, following existing pattern.

@wiredfool
Copy link
Member

This has major conflicts with a patch for a CVE that will be released in the next version.

@wiredfool
Copy link
Member

See #5178

@hugovk
Copy link
Member

hugovk commented Jan 2, 2021

Rebased in #5178.

@hugovk hugovk closed this Jan 2, 2021
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.

Corruption of TIFF images that was introduced in 5.0.0
6 participants