Skip to content

Add support for reading TIFFs with PlanarConfiguration=2#5178

Closed
wiredfool wants to merge 12 commits intopython-pillow:masterfrom
wiredfool:4641_rebase
Closed

Add support for reading TIFFs with PlanarConfiguration=2#5178
wiredfool wants to merge 12 commits intopython-pillow:masterfrom
wiredfool:4641_rebase

Conversation

@wiredfool
Copy link
Copy Markdown
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
Copy Markdown
Member Author

wiredfool commented Jan 2, 2021

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
Copy Markdown
Contributor

kkopachev commented Jan 5, 2021

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
Copy Markdown
Member Author

wiredfool commented Jan 9, 2021

@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
Copy Markdown
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
Copy Markdown
Contributor

@nulano nulano Jan 9, 2021

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
uint16 photometric = 0; // init to not PHOTOMETRIC_YCBCR
int isYCbCr = 0;
UINT8 planarconfig = 0;
UINT8 planes = 1;
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@kkopachev kkopachev Jan 9, 2021

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
Copy Markdown
Contributor

@kkopachev kkopachev Jan 9, 2021

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
Copy Markdown
Member Author

@wiredfool wiredfool Jan 10, 2021

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
Copy Markdown
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
Copy Markdown
Contributor

@nulano nulano Jan 11, 2021

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
Copy Markdown
Contributor

kkopachev commented Jan 14, 2021

@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
Copy Markdown
Member Author

wiredfool commented Jan 14, 2021

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

2 more reviewers

@kkopachev kkopachev kkopachev left review comments

@nulano nulano nulano left review comments

Reviewers whose approvals may not affect merge requirements

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