Skip to content

fix(imageio): ignore invalid RowsPerStrip chunking#5300

Merged
lgritz merged 2 commits into
AcademySoftwareFoundation:mainfrom
br0nzu:public-imageio-rowsperstrip-zero
Jul 7, 2026
Merged

fix(imageio): ignore invalid RowsPerStrip chunking#5300
lgritz merged 2 commits into
AcademySoftwareFoundation:mainfrom
br0nzu:public-imageio-rowsperstrip-zero

Conversation

@br0nzu

@br0nzu br0nzu commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

This treats non-positive tiff:RowsPerStrip metadata as absent before using it to align generic scanline read/write chunks. That avoids feeding invalid chunking metadata into round_to_multiple while preserving normal positive RowsPerStrip behavior.

The patch adds regression coverage for an OpenEXR file carrying RowsPerStrip=0 through iinfo --hash, iinfo --stats, and an oiiotool conversion path.

Treat non-positive tiff:RowsPerStrip metadata as absent before using it to align generic scanline read/write chunks.

Add regression coverage for an OpenEXR file carrying RowsPerStrip=0.

Signed-off-by: Dongju Lee <dongpago2@gmail.com>
@lgritz

lgritz commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Where does the negative rps come from in the first place?

The only place that is ever set is in tiffinput.cpp:

    if (!m_spec.tile_width) {
        TIFFGetField(m_tif, TIFFTAG_ROWSPERSTRIP, &m_rowsperstrip);
        if (m_rowsperstrip > 0) {
            // Only set the attrib if a legit value was found in the file
            m_spec.attribute("tiff:RowsPerStrip", m_rowsperstrip);
            m_rowsperstrip = std::min(m_rowsperstrip, m_spec.height);
        } else {
            // Default if not found is "one strip for the whole image"
            m_rowsperstrip = m_spec.height;
        }
    }

So it looks like it will only be set if it's positive.

OH, I see... it has to come in through an OpenEXR that cleverly includes metadata called "tiff:RowsPerStrip" set to a bad value.

Comment thread testsuite/iinfo/run.py Outdated
Comment on lines +33 to +35
command += run_app (pythonbin + " src/check-rowsperstrip-zero.py "
+ oiio_app("iinfo") + " " + oiio_app("oiiotool"),
silent=True)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we really need an extra .py file (code to maintain) instead of just directly running the two iinfo and one oiiotool tests here like we would for any other test? Are all of them necessary, or is any one enough to test that this issue has not regressed?

And I'm wondering about whether the test for this fix really fits best here in the "iinfo" test (it's not really testing an iinfo feature in particular) or if there is a different testsuite directory where it makes more sense.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz

lgritz commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

I took the liberty of pushing an amendment to your PR that makes a separate test (and somewhat simpler) header of putting it into the iinfo test. If you think that is ok, I am prepared to merge it.

@br0nzu

br0nzu commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

LGTM

@lgritz lgritz merged commit 21a293a into AcademySoftwareFoundation:main Jul 7, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants