Orient every frame of an animated Imagick image

[nlemoine]

Drivers\Imagick\Modifiers\OrientModifier::apply() calls rotateImage() and flopImage() straight on $image->core()->native() with no iteration. Those act on the frame the iterator is currently pointing at, so an animated source comes back with one frame turned and every other frame as it was.

Repro, a 2 frame 40x20 GIF with orientation 6 set on both frames:

$img = $manager->decodeBinary($blob);
$native = $img->core()->native();
foreach ($native as $f) { $f->setImageOrientation(Imagick::ORIENTATION_RIGHTTOP); }
$native->resetIterator();
$img->orient();

frame 0 20x40 orientation=1
frame 1 40x20 orientation=6

So a mixed geometry sequence, and the frames that were skipped keep their original orientation flag because the setImageOrientation(TOPLEFT) at the end lands on the same single frame. Encoded back to GIF it composes wrong, frame 1 came back showing frame 0's background instead of its own color.

The fix moves the transform into the foreach ($image as $frame) loop the other modifiers already use, and marks each frame aligned instead of just one. After the patch both frames are 20x40 with orientation 1, and the re-encoded GIF reads blue over the whole of frame 1.

getImageOrientation() is a current image call too, so I made it read the first frame explicitly. It was already positional before, but it only mis-turned one frame back then. Now that the value drives the whole sequence a stale read turns everything the wrong way. Measured on a source with mixed per frame flags, orient() gave 4x2 or 2x4 for the same input depending on where the iterator sat, and it is stable at 2x4 after. That path is not reachable through the decoders as far as I can tell, coalesceImages() leaves the iterator at 0, so this is defensive.

I looked at whether this needs RotateModifier's setImagePage(0, 0, 0, 0) and it does not. That guard is for the negative page offsets of non right angles (measured 6x6+-1+-2 at 45 degrees), and orient only ever uses 90, 180 and 270, which all come out +0+0. Encoding an oriented animation with and without the reset gave byte identical GIF and animated AVIF.

Worth saying that the input is awkward to build, GIF carries no EXIF so the orientation has to be set in memory on each frame. That is probably why this went unnoticed.

Two tests added, one for a rotating orientation (6) checking the geometry and the flag on every frame, one for the mirror only orientation (2) on frames that are red on the left and blue on the right. AutoOrientationTest covers the single frame cases for all eight orientations on both drivers and still passes.

One note on writing the second test. I first read the mirrored pixel through $frame->toImage($driver)->colorAt(0, 0) and it went green on develop with the bug in place, because on Imagick toImage() hands back the whole sequence and colorAt() then answers frame 0 (that is #1518). Reading through colorAt($x, $y, $frame) on the original image works, it goes through frame($i)->native() and reads before anything moves the wand.

Same shape as #1517, which was the colorspace modifier missing the same loop.

[olivervogel]

Thank you.

[nlemoine]

Thanks Oliver!