Problem/Motivation

It would be useful to have a core Image::getExif() method to enable fetching EXIF information from image files that contain such metadata, so to avoid custom/contrib to reinvent a way to get it in each implementation. Actually such a method should just hand over execution to the toolkit. A base implementation in ImageToolkitBase seems feasible.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 2630242-11.patch32.38 KBmondrake
#11 interdiff_9-11.txt1.09 KBmondrake
#9 2630242-9.patch32.39 KBmondrake
#9 interdiff_2-9.txt15.78 KBmondrake
#3 2630242-2.patch29.12 KBmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Provide methods to retrieve EXIF image information from the Image object » Provide methods to retrieve EXIF image information via the Image object
Assigned: Unassigned » mondrake
Issue tags: +API addition

Working on a patch.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Active » Needs review
StatusFileSize
new 2630242-2.patch29.12 KB

A first patch. Thanks for any review :)

slashrsm’s picture

slashrsm commented
Status: Needs review » Needs work

Yup, it totally makes sense to have API for this in core. I have few comments: 

  1. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
@@ -63,6 +63,27 @@ public function getMimeType();
+   *   The status of the EXIF information.

    It is not entirely clear what this status means. Let's try to make documentation a bit more clear.

    Return value is of type int. What do different values mean? 

  2. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
@@ -104,6 +117,84 @@ public function getSource() {
+      return $tag ? NULL : $this->exifData;

    If we reach this point something probably went wrong with the parsing process. We should always return NULL in that case, right? 

  3. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
@@ -104,6 +117,84 @@ public function getSource() {
+    if ($exif_data = @exif_read_data($this->getSource())) {

    Why @? 

  4. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
@@ -132,6 +162,39 @@ public function getWidth();
+  public function setExifData(array $exif_data, $exif_status = NULL);

    Is this meant to be used by anything outside toolkit class? If not then let's not make it public.

    It is a bit confusing to have set method and no way to save that data into the image file. When I initially saw this function I assumed it is there to allow one to change EXIF info, which is not the case.

  5. Adding functions to existing interfaces is breaking BC. We need to figure out how to implement this in a non BC-breaking way.
mondrake’s picture

mondrake
Location 🇮🇹
commented

Thanks for reviewing @slashrsm!

Let's address point 5 before anything else, I think this is the key point:

Adding functions to existing interfaces is breaking BC

Really? In Allowed changes during the Drupal 8 release cycle I find, in the allowed changes for minor releases,

new APIs or API improvements with backward compatibility (BC)

Wouldn't this fit the definition? This is introducing an API improvement in existing API but it does not touch existing code or method signatures, only adds new methods. I could not find an indication that adding methods to interfaces will be a BC break. On the contrary, in the indications about introducing BC layers, the docs say:

For example, if a method has a badly misleading name, a BC layer can be preserved by leaving the badly-named method as a wrapper for the new one and marking it deprecated.

i.e. you can have a new method as long as you also keep the old one.

slashrsm’s picture

slashrsm commented

Adding a function to an interface is a BC-breaking change. If there is a custom/contrib module that implements that interface it will stop working after such change.

There are ways around this. People usually mention creating new interface that extends the original one.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Yep, you're right... did not think about that. I will rework with new interfaces.

Thanks

mondrake’s picture

mondrake
Location 🇮🇹
commented

Ok, new patch.

#4.5 - Moved the new methods to a ImageExifInterface and a ImageToolkitExifInterface. Now Image implements ImageExifInterface besides ImageInterface, and ImageToolkitBase implements ImageToolkitExifInterface besides ImageToolkitInterface.
Since GDToolkit extends from ImageToolkitBase, it gets the new methods too.
Any toolkit implementing ImageToolkitInterface, but not extending from the base class, will have to implement EXIF related methods on its own.

#4.1 - Added docs.
#4.2 - OK.
#4.3 - Because exif_read_data may throw PHP notices in case of file parsing errors (e.g. zero size files). So we suppress errors from the call, but keep a parsing error condition in the $exifStatus property with value ImageToolkitExifInterface::EXIF_PARSE_FAILURE. We do something similar for getimagesize in the GDToolkit.
#4.4 - No that method is not meant to update info in the source file, but just its in-memory copy in the Image object. It must be public because ImageToolkitOperations may need to update that value. Two cases I know about:
a) you get an Image object from the source file, then get its Exif data, then call a CreateNew operation - in that case the operation need to reset EXIF data because it will be irrelevant to the new image
b) you apply an EXIF autorotate operation to set an image in its right visual orientation based on the EXIF 'Orientation' tag. Once done, you need to set the 'Orientation' tag appropriately (i.e. to '1') so to keep the image resource in sync with the EXIF tag (and avoid that re-calling the operation leads to wrong visualization).

There's no standard PHP function to write back EXIF data to an image file. I googled a bit and found a PEL (PHP EXIF Library) that could do that, but that would be a completely separate issue.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_2-9.txt15.78 KB
new 2630242-9.patch32.39 KB

... and the patch

Status: Needs review » Needs work

The last submitted patch, 9: 2630242-9.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_9-11.txt1.09 KB
new 2630242-11.patch32.38 KB

Fixed test.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

I know very little about EXIF and maybe some questions or comments will look naive but this can be also a point for a good review :) Before doing a review some general comments, concerns, questions:

  • From https://en.wikipedia.org/wiki/Exchangeable_image_file_format I understand that EXIF covers also some audio file formats. Doesn't make sense then to make this interface more abstract? I mean, at least, as naming and location (didn't dig into methods yet).
  • Are there alternatives to EXIF metadata standard? As I understand, EXIF started as proprietary specification so, probably, there might be other ways to store metadata. If the answer is YES, it will have some consequences on the architecture of our implementation. That would mean, we need to provide an abstract layer and let such standards to plug-in, most probable as plugins. And, yes, in the same patch we will ship Drupal with EXIF plugin. In the same time we let contrib to plug other alternatives on board. I don't know if there are other standards on the market but we have to check this before hardcoding EXIF in core.
mondrake’s picture

mondrake
Location 🇮🇹
commented

Interesting questions @claudiu.cristea :)

I am also not an expert here, the proposal here comes from my realization that different contrib modules all have their own implementation of Exif data parsing, all using the same exif_read_data PHP function from the 'exif' PHP extension. I know about:

  • ImageMagick toolkit module
  • Media Entity Image module, where exif_read_data is used to fetch information from the file that is then stored in the entity storage
  • Image Effects (sandbox), where GD autorotate operation and Autorotate image effect both invoke exif_read_data

I am sure there are other examples.

To your points:

  • Alternative to Exif? Well, there's a world out there :) but my general understanding is that these are technically 'extensions' to Exif i.e. at least for JPEG/TIFF these info would be found within some Exif section/tags as returned by exif_read_data
    • ExifTool provides an extensive set of features to read, write and edit metadata in different formats (Perl library)
    • PEL is a PHP library that allows reading and writing Exif headers in JPEG and TIFF images using PHP
  • Not sure about Exif for audio files - we do not have any specific support for audio files in core as we have for images.
  • I like your idea to have 'something pluggable' here.
    • To some extent, this patch here is already providing a 'pluggable' solution, in the sense that the code here is linked to a toolkit which is a plugin. Someone not willing to use exif_read_data could implement an image toolkit derivative starting from GD that, for example, uses the PEL library instead, and may also allow to write back Exif data to the source file which the standard PHP exif extension does not provide
    • But in general - yes - it could make sense to have something even more generic like a 'EmbeddedMetadataFileInterface' (?) that could provide methods to read/change/write metadata from/to files at Uri (but also from/to objects in memory that represent such files - same distinction as an image file and an ImageInterface object), and plugins that implement e.g. Exif from a JPEG file, ID3 from a MP3 file, or whatever.

I agree, let's discuss further what best architecture should be in place to support this

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue tags: +D8 Media

As a first look, I think that we need to make first an inventory of all kind of standards that are embedding meta-information into the file itself. Then imagine an interface that fits all those possible cases, even it's very abstract. Yes, this would be more a file issue than an image. At the end, we just ship a plugin for EXIF.

Also we need to look if this was not already implemented in contrib. I'm thinking to https://github.com/drupal-media/file_entity and other Media modules. Don't have time right now but I think the Media team should be involved in this discussion.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: -D8 Media +D8Media

Tag typo

slashrsm’s picture

slashrsm commented

Media team is involved in this discussion. file_entity doesn't handle that. media_entity_image does some of this, but since it is not the only storage component we have would make a lot of sense to have something that everybody can use.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Started a possible contrib module here https://github.com/mondrake/file_mdm. Any input appreciated.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Active

I have published on d.o a contrib module File metadata manager that is introducing a service and API to manage this, still in development. A module implementing an EXIF metadata plugin is in progress and will follow.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Closed (won't fix)
Issue tags: +Media Initiative

I suggest to close this as won't fix. At least the approach of this patch, to get EXIF via the Image object, will not happen.

In the contrib space, the File metadata manager module is now in beta, and a first implementation proposed for ImageMagick at #2749283: Enable caching of ::parseFile results to reduce file I/O and shell calls. Reviews there appreciated :)

That will use File metadata manager and a plugin implemented in the ImageMagick module to retrieve and cache image information via ImageMagick's 'identify' executable.

A plugin to manage EXIF information is in wip at EXIF metadata plugin. That one could be used in the ImageMagick and the Image Effects modules that need access to image EXIF information.