Add new_from_memory / write_to_memory #13

kleisauke · 2017-09-09T13:49:44Z

Implementation of new_from_memory / write_to_memory. It can wrap an image around a memory array and write an image to a memory array.

However there seems to be something wrong with the passing the zval array to vips_image_new_from_memory because the image average returns random(?) values (vips_image_new_from_memory_copy doesn't fix this). Therefore I'm not sure whether writing to memory works (how should I test this without doing new_from_memory first?).

~~Next week I'll see if I can finish this. I've almost no experience in writing PHP extensions (and in general the C programming language) so my apologies if it takes longer.~~

^{(I've selected the Allow edits from maintainers option that allows you to make commits on this branch)}

Edit:
All done! 🎉

Work in progress!

jcupitt · 2017-09-09T13:56:29Z

Oh nice! I've almost got the JS binding working, I'll have a look this this when I'm done with that.

kleisauke · 2017-09-09T20:55:22Z

OK, all done! I had to convert the HashTable array to a regular integer array. Also vips_image_new_from_memory_copy was needed to make the image average working.

kleisauke · 2017-09-09T21:01:33Z

vips.c

+	zval *IM;
+	VipsImage *image;
+	size_t arr_len;
+	uint8_t *arr;


Can we expect larger values than 255? If so, then this should be a regular int.

It's always bytes there, so it's fine.

kleisauke · 2017-09-09T21:04:10Z

vips.c

+		RETURN_LONG(-1);
+	}
+
+	if ((format_value = vips_enum_from_nick("enum", VIPS_TYPE_BAND_FORMAT, format)) < 0) {


Should I change enum (the domain) to php-vips?

Yes, good idea.

jcupitt · 2017-11-21T12:16:37Z

vips.c

+
+	int i;
+	for (i = 0; i < arr_len; i++) {
+		add_next_index_long(return_value, arr[i]);


A byte array will be horribly slow: you have to loop for each pixel.

I would use a binary string, I think. This is what the libvips bindings for python / ruby / javascript / lua etc. all do anyway.

jcupitt · 2017-11-21T12:17:22Z

vips.c

@@ -1389,6 +1388,93 @@ PHP_FUNCTION(vips_image_copy_memory)
 }
 /* }}} */

+/* {{{ proto resource vips_image_write_to_memory(array data, integer width, integer height, integer bands, string format)


typo: that should be vips_image_new_from_memory, of course.

jcupitt · 2017-11-21T12:19:26Z

vips.c

+	band_format = format_value;
+
+	const int size = zend_hash_num_elements(ht);
+	int arr[size];


This will be slow for large images, and perhaps break the stack. I think we should use binary strings to swap images back and forth. You can skip this expensive copy operation.

jcupitt · 2017-11-21T12:28:50Z

Hello, it all looks fine, I think there's just an issue with how we represent memory arrays. I think actual PHP arrays will be slow, and we should use binary strings.

maybe once libvips/php-vips-ext#13 is done, we can use that

kleisauke · 2017-11-21T15:15:16Z

I've changed the PHP arrays to binary strings with commit 2032b9f. Let me know what you think.

kleisauke · 2017-11-21T16:04:06Z

tests/037.phpt

+<?php if (!extension_loaded("vips")) print "skip"; ?>
+--FILE--
+<?php
+  $binary_str = pack("c*", ...array_fill(0, 200, 0));


Should I use "C*" (unsigned char) here instead?

I'm not sure. Probably, yes.

To match the 0 to 255 range.

maybe once libvips/php-vips-ext#13 is done, we can use that

jcupitt · 2017-11-21T16:55:05Z

vips.c

-	for (i = 0; i < arr_len; i++) {
-		add_next_index_long(return_value, arr[i]);
-	}
+	RETVAL_STRINGL((char *)arr, arr_len);


RETVAL_STRINGL will take a copy of the string, so we need a g_free(arr); afterwards to avoid a leak (I think).

Fixed in 2880405.

maybe once libvips/php-vips-ext#13 is done, we can use that

jcupitt · 2017-11-22T20:07:42Z

\o/

chregu · 2017-11-23T08:43:58Z

Cool. But I can't figure out how to make imagick read it.

$image = vips_image_new_from_file($file);
$bin = vips_image_write_to_memory($image['out']);

file_put_contents("/tmp/foo.vips", $bin);
$imagick = new \Imagick();
$imagick->readImage("/tmp/foo.vips");

Complains about

Fatal error: Uncaught ImagickException: improper image header `/tmp/foo.vips' @ error/vips.c/ReadVIPSImage/412 in /vagrant/testimages/convert.php:14

No idea, what kind of header it needs, I randomly tried the two at
https://github.com/ImageMagick/ImageMagick/blob/master/coders/vips.c#L149
but then it complains about

Fatal error: Uncaught ImagickException: width or height exceeds limit `/tmp/foo.vips' @ error/cache.c/OpenPixelCache/3484 in /vagrant/testimages/convert.php:14

any pointers? Or is that not supposed to be used for this? (my imagick should be able to read vips images)

jcupitt · 2017-11-23T08:47:48Z

write_to_memory just outputs the raw pixels, so for a 2x2 pixel RGB image it would output 12 bytes;

RGBRGB
RGBRGB

You'd need to use an imagick thing that could wrap an image around a raw byte array.

chregu · 2017-11-23T09:58:43Z

Got it working, but maybe not worth the trouble compared to just using tiffsave (but I should benchmark it ;))
That's the code

$image = \Jcupitt\Vips\Image::newFromFile($file);
$image = $image->colourspace(\Jcupitt\Vips\Interpretation::RGB16);
$bin = vips_image_write_to_memory($image->image);
$imagick = new \Imagick();
$imagick->setSize($image->width, $image->height );
$imagick->setFormat("rgb");
$imagick->readImageBlob($bin);
$imagick->setFormat("png");
$imagick->writeImage("foo.png");

(made $image->image public to make the example code easier ;))
Had to convert to RGB16 since my imagick is compiled with Q16. Couldn't figure out, how to tell imagick, that it's only 8bit... And $imagick->setImageDepth(8); is not working at that point of the lifecycle

jcupitt · 2017-11-23T13:44:21Z

Very nice! I pasted your code here:

https://github.com/jcupitt/php-vips/blob/master/examples/vips-magick.php

and it seems to work well!

I'll see if I can get it working with some other image types (mono, RGBA, CMYK etc etc).

Yes, it has to be 16 bit. IM represents everything internally as whatever your quantum is set to, and you can't change it except by recompiling the whole thing. It's one of the reasons IM is a bit sluggish.

IM7 makes you use float (four bytes) everywhere, I think, so it's 2x slower again.

jcupitt · 2017-11-23T15:00:02Z

I tried a benchmark:

#!/usr/bin/env php
<?php

require __DIR__ . '/../vendor/autoload.php';

use Jcupitt\Vips;

/* Benchmark passing images from libvips into magick via either an uncompressed
 * buffer or a formatted file.
 */

$image = Vips\Image::newFromFile($argv[1]);
// find the image average ... this will force the whole image into memory, so
// we don't time any of the loading
echo "image average: " . $image->avg() . "\n";

// time converting to IM via a memory buffer
$start = microtime(TRUE);
$x = $image->colourspace(Vips\Interpretation::RGB16);
$bin = $x->writeToMemory();
$imagick = new \Imagick();
$imagick->setSize($x->width, $x->height);
$imagick->setFormat("rgb");
$imagick->readImageBlob($bin, "rgb");
$end = microtime(TRUE);
echo "importing into imagick via a memory buffer: " . ($end - $start) . "s\n";

// time converting to IM via a TIFF-formatted memory buffer
$start = microtime(TRUE);
$bin = $x->writeToBuffer(".tif");
$imagick = new \Imagick();
$imagick->readImageBlob($bin);
$end = microtime(TRUE);
echo "importing into imagick via a tiff: " . ($end - $start) . "s\n"; 

/*
 * Local variables:
 * tab-width: 4
 * c-basic-offset: 4
 * End:
 * vim600: expandtab sw=4 ts=4 fdm=marker
 * vim<600: expandtab sw=4 ts=4
 */

With a 10k x 10k RGB jpg I see:

$ ./vips-magick-bench.php /data/john/pics/wtc.jpg 
image average: 117.84864125648
importing into imagick via a memory buffer: 0.7385721206665s
importing into imagick via a tiff: 0.97380518913269s

So memory is maybe 30% faster.

chregu · 2017-11-23T15:25:48Z

great, thanks for that. Will then maybe integrate it later

maybe once libvips/php-vips-ext#13 is done, we can use that

Implement new_from_memory / write_to_memory

4f5f49f

Work in progress!

kleisauke added 2 commits September 9, 2017 22:40

Finish implementation

0b42b22

Fix indentation

8c7ecb7

kleisauke changed the title ~~Add new_from_memory / write_to_memory (WIP)~~ Add new_from_memory / write_to_memory Sep 9, 2017

kleisauke commented Sep 9, 2017

View reviewed changes

Fix return type of new_from_memory

1eeadec

kleisauke mentioned this pull request Nov 21, 2017

The fastest way to convert from vips to imagick? #16

Closed

jcupitt reviewed Nov 21, 2017

View reviewed changes

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 21, 2017

getImageStringForLoad and use tiff for converting to imagick

4aa02e2

maybe once libvips/php-vips-ext#13 is done, we can use that

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 21, 2017

getImageStringForLoad and use tiff for converting to imagick

2e4fd7b

maybe once libvips/php-vips-ext#13 is done, we can use that

Incorporate review

2032b9f

kleisauke commented Nov 21, 2017

View reviewed changes

Change to unsigned char

c347899

To match the 0 to 255 range.

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 21, 2017

getImageStringForLoad and use tiff for converting to imagick

b2a3d4b

maybe once libvips/php-vips-ext#13 is done, we can use that

jcupitt reviewed Nov 21, 2017

View reviewed changes

Avoid a memory leak

2880405

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 22, 2017

getImageStringForLoad and use tiff for converting to imagick

5d29a9c

maybe once libvips/php-vips-ext#13 is done, we can use that

jcupitt merged commit fc56cff into libvips:master Nov 22, 2017

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 27, 2017

getImageStringForLoad and use tiff for converting to imagick

76c5077

maybe once libvips/php-vips-ext#13 is done, we can use that

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 28, 2017

getImageStringForLoad and use tiff for converting to imagick

d12b334

maybe once libvips/php-vips-ext#13 is done, we can use that

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Dec 9, 2017

getImageStringForLoad and use tiff for converting to imagick

76b9bd9

maybe once libvips/php-vips-ext#13 is done, we can use that

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 20, 2018

getImageStringForLoad and use tiff for converting to imagick

42b9291

maybe once libvips/php-vips-ext#13 is done, we can use that

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 20, 2018

getImageStringForLoad and use tiff for converting to imagick

b91338c

maybe once libvips/php-vips-ext#13 is done, we can use that

chregu added a commit to rokka-io/imagine-vips that referenced this pull request Nov 20, 2018

getImageStringForLoad and use tiff for converting to imagick

3d90c96

maybe once libvips/php-vips-ext#13 is done, we can use that

Add new_from_memory / write_to_memory #13

Add new_from_memory / write_to_memory #13

Uh oh!

Conversation

kleisauke commented Sep 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcupitt commented Sep 9, 2017

Uh oh!

kleisauke commented Sep 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcupitt commented Nov 21, 2017

Uh oh!

kleisauke commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcupitt commented Nov 22, 2017

Uh oh!

chregu commented Nov 23, 2017

Uh oh!

jcupitt commented Nov 23, 2017

Uh oh!

chregu commented Nov 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcupitt commented Nov 23, 2017

Uh oh!

jcupitt commented Nov 23, 2017

Uh oh!

chregu commented Nov 23, 2017

Uh oh!

Uh oh!

kleisauke commented Sep 9, 2017 •

edited

Loading

chregu commented Nov 23, 2017 •

edited

Loading