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.

Sign up for GitHub

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

Jump to bottom

Add support for reading TIFFs with PlanarConfiguration=2 #5178

Closed
wants to merge 12 commits into from
Closed

Add support for reading TIFFs with PlanarConfiguration=2 #5178

wants to merge 12 commits into from

Conversation

wiredfool
Copy link
Member

@wiredfool wiredfool commented Jan 2, 2021 •
edited by hugovk
Loading

@wiredfool wiredfool mentioned this pull request Jan 2, 2021
Closed
@hugovk hugovk changed the title Rebase of #4641 Add support for reading TIFFs with PlanarConfiguration=2 Jan 2, 2021
@wiredfool
Copy link
Member Author

I'm afraid I don't have the setup to debug the windows side of this at the moment.

@radarhere radarhere added the TIFF label Jan 2, 2021
@kkopachev
Copy link
Contributor

I am not familiar with Windows specifics here, but here is some observations:
Current code failed only on x86. On few runs it died with 'Access violation' error and on Python 3.9 it throws Col out of range, max 99 error. I thought it's something wrong in calculation of accessing tile width and enabled TRACE to see what's up - and x86 runs all successful, but now it failed on all x64 in different test.
Thought that might be helpful.

@wiredfool
Copy link
Member Author

@nulano Do you have any insight into this?

nulano
nulano reviewed Jan 9, 2021
View reviewed changes
src/libImaging/TiffDecode.c
Comment on lines +634 to +637
shuffler((UINT8*) im->image[tile_y + y] + x * im->pixelsize,
state->buffer + current_line * row_byte_size,
current_tile_width
);
Copy link
Contributor

@nulano nulano Jan 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not yet sure what is going on, but compiling with debug information shows that state->buffer is NULL on this line even though it was valid on line 587 for the realloc call.

The error doesn't show up when compiling with in a debug build (setup.py build_ext --dubug), I had to change setup.py line 852 to Extension("PIL._imaging", files, extra_compile_args=["/Z7"], extra_link_args=["/DEBUG"]), and copy the PDB to the PIL directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Solved it, see wiredfool#6

@nulano nulano mentioned this pull request Jan 9, 2021
Merged
nulano
nulano reviewed Jan 9, 2021
View reviewed changes
src/libImaging/TiffDecode.c Outdated
@@ -405,6 +402,11 @@ ImagingLibTiffDecode(
TIFF *tiff;
uint16 photometric = 0; // init to not PHOTOMETRIC_YCBCR
int isYCbCr = 0;
UINT8 planarconfig = 0;
UINT8 planes = 1;
Copy link
Contributor

@nulano nulano Jan 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@kkopachev Is there any reason to use UINT8 for planes? It looks to me like it would make more sense to use int.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, should be just int 🤦

kkopachev
kkopachev reviewed Jan 9, 2021
View reviewed changes
src/libImaging/TiffDecode.c Outdated
// if number of bands is 1, there is no difference with contig case
if (planarconfig == PLANARCONFIG_SEPARATE &&
im->bands > 1 &&
photometric != PHOTOMETRIC_YCBCR) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
photometric != PHOTOMETRIC_YCBCR) {
!isYCbCr) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @nulano and @kkopachev

nulano
nulano reviewed Jan 11, 2021
View reviewed changes
Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

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

Same as UINT8 planes -> int planes.

src/libImaging/TiffDecode.c Outdated
state->errcode = IMAGING_CODEC_BROKEN;
return -1;
}
UINT8 plane;

This comment was marked as resolved.

Sign in to view

src/libImaging/TiffDecode.c Outdated
* backwards
*/

UINT8 plane;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
UINT8 plane;
int plane;

@kkopachev
Copy link
Contributor

@wiredfool I have some changes I did on top of this PR to fix BigEndian skipped tests and a bit of refactoring. Do you want me to create PR into this branch or wait until this merged and open into Pillow:master afterwards for separate review?
changes are here: wiredfool/Pillow@4641_rebase...kkopachev:4641_rebase

@wiredfool
Copy link
Member Author

Might as well pr to this branch

@kkopachev kkopachev mentioned this pull request Jan 14, 2021
Closed
@wiredfool wiredfool mentioned this pull request Mar 28, 2021
Merged
@wiredfool wiredfool closed this Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nulano nulano nulano left review comments

@kkopachev kkopachev kkopachev left review comments

Assignees
No one assigned
Labels
Needs Rebase TIFF
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wiredfool @kkopachev @nulano @radarhere