[FEATURE] Introduce image metadata

[afoeder]

This introduces metadata as a gettable property of
the ImageInterface and its implementations, in order
to carry initial image metadata along the lifecycle.

The distributed implementation determines the metadata
by the image's EXIF information, but it not limited to
this in the future.

This is a preparation for #222 and related to #190.

[staabm]

Maybe better defer reading the exif data until actually needed?

[stof]

@staabm the AbstractImage is instantiating the ExifMetadata only when you retrieve it

[staabm]

Actually it is only needed when getOrientationis called not yet when this object is created

[afoeder]

@stof got it right at least as far as I intended how it should work; the problem with a getOrientation lazy-load is IMO the fact that it's rather unnice, because when later that model matures, with other getters, you would need an initialization step every time.
But I'll push a follow-up with that.

[stof]

I think the metadata should be reset when the image gets modified.

and it should also be reset when cloning

[stof]

Oh, and please fix the coding standards according to PSR-2 (the PHP-CS-Fixer tool should be able to fix most of it for you)

[afoeder]

@stof ,

I think the metadata should be reset when the image gets modified.
and it should also be reset when cloning

ok, will do.

[afoeder]

@stof, I decided to not reset it on cloning and modifying, but to clone the metadata if the image itself is cloned.
Resetting doesn't make sense IMO because the metadata is still the "same" on a cloned image.
And keeping the metadata "up to date" should be the responsibility of the implementations resp. the implementors.

[romainneutron]

Just back to keyboard, I need a bit time to review this, thanks for your contribution !

[romainneutron]

Well, I do think metadata should be key/value informations about the state of the image when opening it.
What about implementing ArrayAccess so we could read data easily without implementing any dedicated method.

I've also a note about reading metadata from with exif_read_data('data://image/jpeg;base64,' . base64_encode($this->image->get('jpg')));. This won't return anything in case ImageInterface::strip would have been called previously, because it removes any exif informations.

I was thinking about reading metadata before the Image object is created, and inject the Metadata object at constructor, but it adds an overhead that should be benchmarked to have get an idea about how much it is.
Injecting metadata properties at constructor has a nice side effect : we would now know what's the original image path in a standardize way (see https://github.com/avalanche123/Imagine/blob/develop/lib/Imagine/Gd/Image.php#L61, path is injected at constructor to allow calling ImageInterface::save without arguments).

What's your opinion about this ?

[afoeder]

This won't return anything in case ImageInterface::strip would have been called previously, because it removes any exif informations.

Argh now I know why I had it designed that way on the first draft: exactly that's why ;)
I intended to initialize the metadata on constructing the image and being available on the whole lifecycle of the image, changing its properties accordingly when that makes sense (i.e., changing the orientation value for example when rotating or flipping the image).

So, since I initially intended to prepare most on constructing, I'm 👍 on that.
However, the exif_read_data('data://image/jpeg;base64,' . base64_encode($this->image->get('jpg'))); looks like overhead by reading (the whole image would be base64encoded, attached to a string and read again, that must consume memory as hell, I think.
Is there a smarter way to get the exif data on Image constructing?

[romainneutron]

If the file is a filesystem file : exif_read_data('/path/to/file'), otherwise exif_read_data('data://'.$mimetype.';base64,'.base64_encode($data));

To avoid this kind of overhead in case exif metadata are not used at all, we could think about a metadata load strategy with a default implementation to a BasicMetadataStrategy that would not perform any call on image constructor, just get a file path for example.

[afoeder]

If the file is a filesystem file : exif_read_data('/path/to/file'), otherwise exif_read_data('data://'.$mimetype.';base64,'.base64_encode($data));

yes of cource; but that would mean we unfortunately have to add that to every implementation of \Imagine\Image\ImagineInterface::open, ::load and ::read.

I actually wanted to implement a MetadataDetectionStrategy but I didn't find a good solution since Imagine doesn't have some kind of configuration or so.

(//edit: btw, why is the IRC channel that empty? :-( )

[romainneutron]

Updating \Imagine\Image\ImagineInterface::open, ::load and ::read is not a problem as default strategy would be seamless (only load the filepath (if provided) as the only metadata).

Are you okay to work on such strategy implementation ?
I can fork your branch and do it if you need some help

[afoeder]

@romainneutron but what would decide what actual loading strategy to use, or, whether exif or "not"?

[romainneutron]

I've propose a MetadataReaderInterface in https://github.com/afoeder/Imagine/pull/1

This solves the issues mentioned above. Using exif metadata reader is easy :

$imagine = new Imagine();
$imagine->setMetadataReader(new Imagine\Image\Metadata\ExifMetadataReader());

A reader returns a MetadataBag that implements ArrayAccess and is injected at image construction. Feedback appreciated

[staabm]

in case the image is big, this will use a lot of memory (base64 will use 33% more memory than the image has in size).
Would it make sense to dump the image to a file (if it is not already one) and let exif_read_data read from the file? This would not eat php's memory, but might be slower.

[staabm]

another alternative would be to use a stream-filter for base64-encoding. this will help against memory limitations without the need to dump the file.. use something like the following with an in-memory stream:
stream_filter_append($fh, 'convert.base64-encode');

[romainneutron]

yep, that's right.

I've propose an enhancement on top of this PR (see https://github.com/afoeder/Imagine/pull/1) that address this comment (in case ImagineInterface::open is used)

[romainneutron]

Your proposition is smart. I'll fix my PR with it

[romainneutron]

@staabm I've tried your proposition, but as long as exif_read_data does not accept streams, the data URI must be computed before calling this function. I can't figure how we could you stream_filter_append as you mentioned. See :

$stream = fopen('php://memory','r+');
fwrite($stream, $data);
rewind($stream);
stream_filter_append($stream, 'convert.base64-encode', STREAM_FILTER_READ);

exif_read_data('data://image/jpg;base64,' . stream_get_contents($stream));

[staabm]

@evert any idea whats going on in the example above?

[evert]

Using the stream filter doesn't make much sense here. You're not saving any memory by letting a stream filter do the encoding, rather than base64 encode, because the input ($data) and the output (the data:// url) are still strings.

However, i think the reason the stream filter is failing, is because you probably need to use STREAM_FILTER_WRITE instead.

[staabm]

Ohhh right, it won't save memory until exif_read_data will accept a stream.... Will try your fix nevertheless, thanks.

[staabm]

tested it now and it works when used like this:

$data = str_repeat('lalaaaa', 5000);

$stream = fopen('php://memory','r+');
stream_filter_append($stream, 'convert.base64-encode', STREAM_FILTER_WRITE);
fwrite($stream, $data);
rewind($stream);

var_dump(stream_get_contents($stream));

I measured memory consumption with the teststring $data = str_repeat('lalaaaa', 5000); and it seems that using the in-memory stream consumes even more memory - I think mainly because of the buffer not beeing free'd yet.

To put it into a nutshell: atm there is no benefit when using streams here. I would leave it as is, until exif_read_data supports streams or someone encounters the problem of insufficient memory and then one could reconsider using a tempfile for this to cut memory consumption by ~75%

[evert]

I'm available for contract work to write a pure-php implemenation for reading exif data using streams ;) should be no more than 5 hours

[romainneutron]

@afoeder What's the status on this PR, I don't find you fork on your profile. Did you gave up ? Can I re-open this PR on my account and finish what we begin ?

[afoeder]

@romainneutron yes, I somehow gave up since I had no actual clue how you meant it to be implemented; and, I seem to have unlearnt how to use vanilla PHP with things like finding interface implementors, DI etc...

[romainneutron]

I did a PR on your repo weeks ago.
Anyway, can I use your code ?

[afoeder]

yep, sure!

[romainneutron]

Replaced by #304