Switch to php ffi

[jcupitt]

This gets rid of php-vips-ext and reimplements everything with the new php-ffi module instead. It should be simpler to develop and support, and much easier to deploy.

It now requires php 7.4 or later (when php-ffi was merged). I've bumped the version to 2.0 since this feels like a large change. The API is the same.

All tests pass, and it seems stable.

There are some obvious things we should still do, but I think they can wait until after this is merged.

• Support preloading, see https://www.php.net/manual/en/class.ffi.php
• Rewrite the enum and doc generator in php.
• Add source/target API
• Add progress callbacks etc.
• Add mutable.

[jcupitt]

Thanks very much!

vips_cache_set_max_mem() isn't supposed to work, that was part of the API from php-vips-ext. Try Vips\Config::cacheSetMaxMem().

I've added findLoad and findLoadBuffer.

[chregu]

Ok, basics work now, I can render an image with our "pipeline"

But still some issues when I run our testsuite

FFI\Exception: Attempt to call undefined C function 'vips_image_write_to_array'

from eg. https://github.com/libvips/php-vips/blob/switch-to-php-ffi/src/Image.php#L1108

and when I run those tests I have some

(process:150): GLib-GObject-WARNING **: 17:14:59.636: instance of invalid non-instantiatable type '(null)'

(process:150): GLib-GObject-CRITICAL **: 17:14:59.636: g_signal_handler_is_connected: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed

(process:150): GLib-GObject-CRITICAL **: 17:14:59.636: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

reporting (need to check what those tests exactly do, but those logs weren't there before)

[jcupitt]

Oop, you're right, writeToArray won't work. I'll have a look, thanks!

[kleisauke]

I left a few installation comments inline. The whole test suite on the (now elderly) 3.x branch of the weserv/images project almost passes with this PR. A few unit tests gave these errors:

Jcupitt\Vips\Exception: 1 arguments required, but 4 supplied

I was able to reproduce that on the php-vips test suite by modifying the testVipsWriteToBuffer() test.

--- a/tests/WriteTest.php
+++ b/tests/WriteTest.php
@@ -52,7 +52,7 @@ class WriteTest extends TestCase
         $filename = __DIR__ . '/images/img_0076.jpg';
         $image = Vips\Image::newFromFile($filename, ['shrink' => 8]);
 
-        $buffer1 = $image->writeToBuffer('.jpg');
+        $buffer1 = $image->writeToBuffer('.jpg', ['strip' => true, 'Q' => 85, 'optimize_coding' => true]);
         $output_filename = $this->tmp('.jpg');
         $image->writeToFile($output_filename);
         $buffer2 = file_get_contents($output_filename);

It seems that it somehow identifies these optional arguments as required ones.

[jcupitt]

I added writeToArray(), plus another test. Thanks!

[jcupitt]

The writeToBuffer() failure is bizarre, I'll have a look.

[jcupitt]

OK, writeToBuffer fixed too. Thanks for the careful review!

[chregu]

Thanks, too. Looks better now, but still have some segfaults running our tests. Need to look into more carefully and try to make it reproducable. But it seems to be some "heisenbug", depending on which tests I run in which order. And messages like these still happen

(process:3061): GLib-GObject-CRITICAL **: 17:57:10.097: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

(process:3061): GLib-GObject-WARNING **: 17:57:10.097: instance of invalid non-instantiatable type '<invalid>'

(process:3061): GLib-GObject-CRITICAL **: 17:57:10.097: g_signal_handler_is_connected: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed

I'll try to be more helpful tomorrow

[jcupitt]

I've probably got some extra or missing free() :( The php-ffi docs are extremely unclear about memory ownership, unfortunately.

[chregu]

Finally could do a self contained script, which reports a GLib-GObject-CRITICAL **: 10:55:16.453: g_object_unref: assertion 'G_IS_OBJECT (object)' failed "error"

Not super short still, but maybe it helps, it's code from all over our codebase

If you comment out $bg = extendImageWithVips($bg, $box, ['x' => 20, 'y' => 20]); or the ->copyMemory() afterwards, it doesn't show the error.

Used with this image https://chregus.rokka.io/or/a2272e/animated.gif

<?php

use Jcupitt\Vips\BandFormat;
use Jcupitt\Vips\BlendMode;
use Jcupitt\Vips\Extend;
use Jcupitt\Vips\Image as VipsImage;

require 'vendor/autoload.php';

$foregroundImages = getImages();
$box = ['width' => 600, 'height' => 600];
$backgroundImage = generateImage($box);
$backgroundImages = [];

$foregroundImagesCount = \count($foregroundImages);
for ($i = 0; $i < $foregroundImagesCount; ++$i) {
    $bg = $backgroundImage;

    $copy = clone $foregroundImages[$i];

    // comment this to remove the error
    $bg = extendImageWithVips($bg, $box, ['x' => 20, 'y' => 20]);
    $bg = $bg->composite([$copy], [BlendMode::OVER])->copyMemory(); // or remove the copyMemory()
    $backgroundImages[] = $bg;
}

$v = joinMultilayer($backgroundImages);
$v->gifsave('foo.gif');

function extendImageWithVips(VipsImage $vips, array $box, array $start)
{
    $new = generateImage($box);
    $vips = $new->insert($vips, $start['x'], $start['y']);

    return $vips;
}

function getImages()
{
    $images = [];
    $vips = \Jcupitt\Vips\Image::newFromFile('animated.gif', ['n' => -1]);
    $page_height = $vips->get('page-height');
    $total_height = $vips->height;
    $total_width = $vips->width;
    for ($i = 0; $i < ($total_height / $page_height); ++$i) {
        $images[$i] = $vips->crop(0, $page_height * $i, $total_width, $page_height);
    }

    return $images;
}

function generateImage(array $size)
{
    $width = $size['width'];
    $height = $size['height'];

    $pixel = VipsImage::black(1, 1)->add([0, 255, 0, 255])->cast(BandFormat::UCHAR);
    // Extend this 1x1 pixel to match the origin image dimensions.
    $vips = $pixel->embed(0, 0, $width, $height, ['extend' => Extend::COPY]);
    $vips = $vips->copy(['interpretation' => \Jcupitt\Vips\Interpretation::SRGB]);

    return $vips;
}

function joinMultilayer(array $images): VipsImage
{
    /** @var \Jcupitt\Vips\Image $vips */
    $vips = $images[0]->copy();
    $height = $vips->height;
    $width = $vips->width;
    $vips->set('page-height', $height);
    $vips->set('gif-loop', 0);

    foreach ($images as $_k => $_v) {
        if (0 === $_k) {
            continue;
        }
        // make frame the same size as the original, if height is not the same (if width is not the same, join will take care of it
        if ($_v->height !== $height) {
            $_v = $_v->embed(0, 0, $width, $height, ['extend' => Extend::BACKGROUND]);
        }
        $vips = $vips->join($_v, 'vertical', ['expand' => true]);
    }

    return $vips;
}

[jcupitt]

Oh that's great @chregu, thanks very much!

I'll see if I can find the issue.

[jcupitt]

Ouch that was painful, but I think I've finally found a fix. Or your program no longer crashes for me at least.

[jcupitt]

You probably know, but just in case, you can replace joinMultilayer with arrayjoin:

$animated = Vips\Image::arrayjoin($images, ['across' => 1])->copy();
$animated->set('page-height', $images[0]->height);
$animated->set('loop', 0);

If you set loop (rather than gif-loop) it'll work correctly for animated webp and heic as well.

[chregu]

The most painful fixes are usually 4 lines or so ;) Thanks a lot, our tests actually run through now. Without warnings. And also thanks for the arrayjoin hint, will try that

[jcupitt]

Great! Let's merge then, and think about the things that still need adding in another PR.

Thanks very much for all the testing and code improvements!

[jcupitt]

Remaining TODOs: #137

[mstaack]

this is just great! thank you