Skip to content

Fix ReverseImages for even image counts, and a 32-bit overflow in ReduceColors - #57

Open
undisker wants to merge 1 commit into
galfar:masterfrom
undisker:fix/two-small-bugs
Open

Fix ReverseImages for even image counts, and a 32-bit overflow in ReduceColors#57
undisker wants to merge 1 commit into
galfar:masterfrom
undisker:fix/two-small-bugs

Conversation

@undisker

Copy link
Copy Markdown

Two small, independent fixes found while maintaining a downstream fork. Happy to split them into separate PRs if you would prefer.

1. TMultiImage.ReverseImages is wrong for an even image count

The loop runs for I := 0 to GetImageCount div 2, which for an even count reaches the middle pair a second time and swaps it back:

[A,B,C,D]
I=0 -> ExchangeImages(0,3) -> [D,B,C,A]
I=1 -> ExchangeImages(1,2) -> [D,C,B,A]   correct so far
I=2 -> ExchangeImages(2,1) -> [D,B,C,A]   undoes it

Expected [D,C,B,A], actual [D,B,C,A].

Odd counts are unaffected — the final iteration is a self-exchange, so it is a harmless no-op rather than a wrong result. (GetImageCount - 1) div 2 is correct for both parities.

2. ReduceColors computes NumPixels in 32 bits

NumPixels is declared Int64, but

NumPixels := Width * Height;

has two Integer operands, so the multiplication is performed in 32 bits and only then widened on assignment. This is exactly the trap described in the PBuffer comment added in 318c312:

When calculating the offset with all 32 bit operands one of them needs to be cast to 64 bit - Pascal won't promote result to 64 bits by itself even if the target variable is 64 bit.

Casting one operand fixes it. Note the two nearby sites already do this (Int64(Width) * Height and NativeInt(Width) * Height); this one looks like it was simply missed in that pass.


Both changes are two lines plus a comment. No behaviour change for images that were already working.

TMultiImage.ReverseImages loops "to GetImageCount div 2". For an EVEN
count that reaches the middle pair a second time and swaps it back:

  [A,B,C,D]
  I=0 -> exchange(0,3) -> [D,B,C,A]
  I=1 -> exchange(1,2) -> [D,C,B,A]   correct so far
  I=2 -> exchange(2,1) -> [D,B,C,A]   undoes it

Odd counts are unaffected (the last iteration is a self-exchange no-op).
Using (GetImageCount - 1) div 2 is correct for both parities.

ReduceColors declares NumPixels as Int64 but computes it as
"Width * Height" with two Integer operands, so the multiplication is done
in 32 bits before the assignment widens it - the exact trap the PBuffer
comment added in 318c312 warns about. Casting one operand fixes it.

Found while maintaining a downstream fork.
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.

1 participant