Skip to content

Allow for IFDRational with negative numerator and zero denominator #8970

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow for IFDRational with negative numerator and zero denominator #8970

wants to merge 2 commits into from
+19 −35

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented May 22, 2025 •
edited
Loading

Resolves #8965. Alternative to #8966

A user has found an image where the ExposureBiasValue signed rational TIFF tag has a negative numerator but a zero denominator.

When this is passed to our IFDRational, it is considered nan,

Pillow/src/PIL/TiffImagePlugin.py

Lines 374 to 375 in 3c71559

if denominator == 0:
self._val = float("nan")

which means it is not less than zero.

>>> from PIL.TiffImagePlugin import IFDRational
>>> IFDRational(-1, 0) 
nan
>>> IFDRational(-1, 0) < 0
False

This means that it isn't detected as something to write as a signed rational.

Pillow/src/PIL/TiffImagePlugin.py

Lines 690 to 692 in 3c71559

assert isinstance(v, IFDRational)
if v < 0:
self.tagtype[tag] = TiffTags.SIGNED_RATIONAL

This PR suggests replacing v < 0 with (float(v.numerator) < 0) != (float(v.denominator) < 0)

@radarhere radarhere mentioned this pull request May 22, 2025
Open
@radarhere radarhere changed the title Allow for IFDRational with negative numerator and zero denominator Allowed for IFDRational with negative numerator and zero denominator May 22, 2025
@radarhere radarhere changed the title Allowed for IFDRational with negative numerator and zero denominator Allow for IFDRational with negative numerator and zero denominator May 22, 2025
@epou
Copy link

epou commented May 23, 2025

The proposed solution is functionally correct, but it requires developers to manually check the numerator and denominator each time. This introduces potential for inconsistency or error, especially if this behaviour needs to be checked on other parts of the code in the future.

It seems the underlying issue, is that the code assumes IFDRational instances behave like floats and are directly comparable. That expectation is natural and intuitive, and likely why the bug slipped in when the previous change was committed. This made me think that it'd make more sense to align the behavior of IFDRational more closely with float semantics. A more mathematically consistent handling of division-by-zero cases would be:

  • Positive numerator / zero denominator → math.inf
  • Negative numerator / zero denominator → -math.inf
  • Zero numerator / zero denominator → 0 (or we could keep using float('nan'))

If we go this route, and implement __float__ as I did in the other pull request (https://github.com/python-pillow/Pillow/pull/8966/files#diff-6ad43f85f1a075181d4d8cfcd97ae27bba1eccf5c3db5a3457160f98218eba6eR405-R407), the comparisons v < 0 made for setting the tag type would behave correctly. This change would also make IFDRational instances more intuitive to work with and directly comparable using standard float logic.

Does this approach make sense to you? I’d be happy to open a PR implementing the -inf/inf logic so you can review it in context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ImageOps.exif_transpose() raises struct.error on invalid EXIF rational
2 participants
@radarhere @epou