Fix ImImagePlugin.seek() frame stride for non-8-bit modes

[chuenchen309]

Changes proposed in this pull request:

• Fix ImImagePlugin.seek() computing the per-frame byte stride as 8 * len(self.mode) — the length of the mode name string used as bits-per-pixel. That only equals the real value when len(mode) matches the band count and the bands are 8-bit (L, RGB, RGBA, CMYK, LA, P). For I;16 (16-bit), I / F (32-bit) and YCbCr (3-byte), the stride was wrong, so seek(frame >= 1) on a multi-frame IM image raised OSError: image file is truncated (I;16, YCbCr) or returned pixels from the wrong offset (I, F).
• Use 8 * bands * itemsize (via ImageMode), which matches Image.new(mode).tobytes() for every IM-supported mode and leaves the six already-correct modes byte-for-byte unchanged.
• Add a parametrized regression test seeking to frame 1 of a 2-frame IM image in the affected modes.

Reachability, stated honestly: this is a low-frequency bug. Pillow's own _save only writes one frame's data, so Pillow cannot itself produce a valid multi-frame IM file — the trigger is a multi-frame IM/IFUNC file from an external tool in one of these less-common modes. The IM reader does advertise multi-frame support (n_frames/seek are wired), so reading such a file with a wrong stride is a real correctness bug, but it is not a hot path. Happy to close if you'd rather not carry it.

[akx]

It's really not clear to a reader why we multiply things with an integer from the last character of a type string :)

[radarhere]

the trigger is a multi-frame IM/IFUNC file from an external tool in one of these less-common modes

Do you have such a file available? Or did you find an IM specification that you were able to refer to for this?

If not, you're saying that this part of our seeking implementation is incorrect... but only this part, and presuming that the rest of it is correct, and creating a new test based on our seeking implementation. It just seems a bit snake-eating-its-own-tail.

[radarhere]

If you're not aware of any software that interacts with this format, I've created #9849 to suggest deprecating it completely.

[chuenchen309]

Answering both parts directly: no, I don't have such a file, and no, I didn't find a specification. The change came out of reading seek() and nothing else. Your note on #9849 says you went looking for a spec too and found nothing unrelated to Pillow, which matches what I ran into.

And your description of the test is fair. It writes the fixture with Pillow and reads it back with Pillow, so it pins the reader against my reading of the reader, not against anything real.

One thing that doesn't need a spec, offered for #9849 rather than for this PR. _save accepts frames=N and writes it into the header, but ImageFile._save only ever emits one frame of pixel data. On 12.3.0, saving an 8×4 image with frames=3:

mode   file size   512 + one frame   n_frames  is_animated   seek(1)
RGB      608           608              3        True        OSError: image file is truncated
I;16     576           576              3        True        OSError: image file is truncated
F        640           640              3        True        OSError: image file is truncated

So Pillow's own writer produces files that its own reader reports as animated and then cannot seek into — in every mode, including the ones where the stride is already correct. The multi-frame read path appears never to have had a producer anywhere in Pillow, which reads to me as an argument for your deprecation rather than for patching the stride.

For the record, the line I changed can't be spec-derived either: 8 * len(self.mode) is eight times the length of the mode name, which lands on the right value only for the six modes whose name length happens to equal the band count at 8 bits. But that's an argument about internal coherence, and with no producer and no specification I don't think it's worth your time. Happy for this to be closed — say the word, or just close it.

[radarhere]

The logic you used to arrive at this PR is fine.

If my PR for deprecating the plugin is accepted, then I would like to not make any changes (that aren't security-related). I don't think it makes sense to modify a deprecated feature, and I don't expect the bugs you're describing to bother anyone because I don't think anyone is using the format.

If my PR isn't accepted, then I'm happy to engage with the two problems you've described.