Conversation
This is a continuation of PR AcademySoftwareFoundation#5468 -- after fixing the TIFF reader, I audited the other readers for the same problem and found some issues in dds, dpx, fits, heif, ico, openexr, ptex. Those readers point themselves at a subimage, parse its header into m_spec, and only then decide whether to accept it -- but on rejection they return false without undoing any of that. The logical position (m_subimage/m_miplevel) and the decoder state no longer match, and this is a problem because seek_subimage() begins with a "we're already there" early out, which will be wrong. The fix is either to give up the current subimage before overwriting anything, or to adopt the new one only once it has passed every check, so that a failed seek leaves the reader on no subimage rather than on a mismatched one. Rejecting a subimage seek should not cost us the open file: a seek to a different, valid subimage still has to work afterwards. A few needing extra explanation: - HEIF's subimage-to-item-id lookup was off by one: m_item_ids[0] is already the primary image, so subimage 1 mapped back to the primary and every non-primary top-level image was unreachable. Found while testing the above, where a "rejected" seek to subimage 1 turned out to be a second successful seek to subimage 0. - Ptex also adopted the new face's resolution before checking that the face has the requested miplevel, so a failed probe left m_faceres and m_spec describing different faces; the miplevel check now comes first. - GIF also fixes a bug where it would not have correctly done a seek to an earlier subimage other than the first. The DICOM reader has the same problem, but untangling it there needs more than a reordering, so it is handled separately. Assisted-by: Claude Code / Claude Opus 5 Signed-off-by: Larry Gritz <lg@larrygritz.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a continuation of PR #5468 -- after fixing the TIFF reader, I audited the other readers for the same problem and found some issues in dds, dpx, fits, gif, heif, ico, openexr, ptex.
Those readers point themselves at a subimage, parse its header into m_spec, and only then decide whether to accept it -- but on rejection they return false without undoing any of that. The logical position (m_subimage/m_miplevel) and the decoder state no longer match, and this is a problem because seek_subimage() usually begins with a "we're already there" early out, which will be wrong.
The fix is either to give up the current subimage before overwriting anything, or to adopt the new one only once it has passed every check, so that a failed seek leaves the reader on no subimage rather than on a mismatched one. Rejecting a subimage seek should not cost us the open file: a seek to a different, valid subimage still has to work afterwards.
A few needing extra explanation:
HEIF's subimage-to-item-id lookup was off by one: m_item_ids[0] is already the primary image, so subimage 1 mapped back to the primary and every non-primary top-level image was unreachable. Found while testing the above, where a "rejected" seek to subimage 1 turned out to be a second successful seek to subimage 0.
Ptex also adopted the new face's resolution before checking that the face has the requested miplevel, so a failed probe left m_faceres and m_spec describing different faces; the miplevel check now comes first.
GIF also fixes a bug where it would not have correctly done a seek to an earlier subimage other than the first.
The DICOM reader has the same problem, but untangling it there needs more than a reordering, so it will be handled in a separate PR.
Assisted-by: Claude Code / Claude Opus 5