Skip to content

Derive PCX plane count from the image, not the line width - #9798

Open
chuenchen309 wants to merge 1 commit into
python-pillow:mainfrom
chuenchen309:fix/pcx-decode-planes
Open

Derive PCX plane count from the image, not the line width#9798
chuenchen309 wants to merge 1 commit into
python-pillow:mainfrom
chuenchen309:fix/pcx-decode-planes

Conversation

@chuenchen309

@chuenchen309 chuenchen309 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Changes proposed in this pull request:

  • When decoding a multi-plane PCX scanline, PcxDecode.c recovered the per-plane stride by guessing the band count as state->bytes / state->xsize. Since state->bytes is planes * stride, that quotient is wrong whenever the
    even-padded stride differs from the width. For a width-3 RGB line it yields 4 bands / stride 3, the plane-compaction step is skipped, and the R/G/B planes stay interleaved — the image decodes to the wrong colours with no error raised. Derive the split from the known band count (im->bands) instead.
  • Add an RGB round-trip regression at a width that triggers the even-stride padding. The existing test_odd uses a 511×511 RGB image, whose width makes the guess land on the right value by luck, so it never caught this.

Scope note: at width 1 an RGB PCX still raises OSError on read — that is a separate problem on the encoder side (it emits only two of the three colour planes), with a different root cause, so I've left it out of this change.


This PR was authored by an AI coding agent (Claude Code) running on this account: the AI found the bug (verified against the real C codec, not a simulation), ran the repro, wrote the test, and wrote this description. The human account holder reviews every change and is accountable for it. The verification is real and re-runnable from the diff. If this isn't the kind of contribution you want, say so and I'll close it.

When decoding a multi-plane (e.g. RGB) PCX scanline, the decoder recovered
the per-plane stride by guessing the band count as state->bytes / xsize.
state->bytes is planes * stride, so when the padded stride differs from the
width (an odd-width line padded to an even stride) that quotient is wrong: for
a width-3 RGB line it gives 4 bands / stride 3, the plane-compaction step is
skipped, and the R/G/B planes stay interleaved. The pixels decode to the wrong
colours with no error raised.

Use the known band count (im->bands) to split the line instead. Added an RGB
round-trip regression at a width that triggers the padding.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@radarhere radarhere added the 🤖-assisted AI-assisted label Jul 19, 2026
@radarhere

radarhere commented Jul 19, 2026

Copy link
Copy Markdown
Member

For the record, the idea was originally introduced in 5ecec7d, and the reason there are even-padded strides is because the PCX specification states that 'BytesPerLine' 'MUST be an EVEN number.'

An alternate fix would be

--- a/src/libImaging/PcxDecode.c
+++ b/src/libImaging/PcxDecode.c
@@ -69,7 +69,7 @@ ImagingPcxDecode(Imaging im, ImagingCodecState state, UINT8 *buf, Py_ssize_t byt
                 stride = state->bytes / state->bits;
             } else {
                 xsize = state->xsize;
-                bands = state->bytes / state->xsize;
+                bands = state->bytes / (state->xsize + (state->xsize % 2));
                 if (bands != 0) {
                     stride = state->bytes / bands;
                 }

However, I think your fix is simpler.

@radarhere

radarhere commented Jul 28, 2026

Copy link
Copy Markdown
Member

The existing test_odd uses a 511×511 RGB image, whose width makes the guess land on the right value by luck

Testing, I find that the only failing scenario that your fix addresses is when the width is exactly 3.

3 pixels in width, plus 1 padding byte, makes a stride of 4.
Times 3 bands is 12 bytes per line.
Divided by 3 pixels in width gives 4 bands, which is incorrect. In short, ((3 + 1) * 3) / 3 is 4.

For every width other than 3, the calculation results in a number between 3 and 4, and so is rounded down to 3, the correct number of bands. So for 511, ((511 + 1) * 3) / 511 gives 3.0058708414872797.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-assisted AI-assisted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants