Skip to content

[BUG]: Image\Adapter\Imagick::processReflection() advances the wrong frame cursor #17510

Description

@niden

Background

The max-level pass over Image surfaced two defects. The second one - processText() passing a boolean $offsetY to annotateImage() - has been fixed and is recorded at the bottom of this document for history. This one remains.

The defect

The first two loops in processReflection() mutate $reflection but use $this->image->nextImage() as the loop condition. The two objects hold independent frame cursors.

$reflection->setIteratorIndex(0);

while (true) {
    $reflection->flipImage();
    $reflection->cropImage($reflection->getImageWidth(), $height, 0, 0);
    $reflection->setImagePage($reflection->getImageWidth(), $height, 0, 0);

    if (true !== $current->nextImage()) {   // <- advances the wrong object
        break;
    }
}

$reflection's cursor is never advanced, so for an image with N frames:

  • Loop 1 applies flipImage()/cropImage() to $reflection frame 0, N times over, instead of once to each frame. An even N cancels the flip entirely. Frames 1..N-1 are never touched.
  • Loop 2 then finds $this->image's cursor already exhausted by loop 1 and breaks after a single pass, so the fade gradient composites onto frame 0 only.
  • Loop 3 does call setIteratorIndex(0) on the cursor it advances first, so it is correct.

The likely intent is $reflection->nextImage() in both loops, but that must be confirmed against ImageMagick's frame semantics - the two loops may also each need their own setIteratorIndex(0) reset.

Single-frame images are unaffected. With N=1, nextImage() returns false on the first call, every loop runs exactly once, and the output is correct. That is the entire reason the suite is green.

Metadata

Metadata

Assignees

Labels

5.0The issues we want to solve in the 5.0 releasebugA bug reportstatus: mediumMedium

Type

Projects

  • Status
    Implemented

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions