Introduced MediaGalleryMetadata services with XMP support #1514

sivaschenko · 2020-06-23T09:57:43Z

Intorudced the AddMetadataInterface and GetMetadataInterface services according to the technical design: https://github.com/magento/adobe-stock-integration/wiki/Technical-Design:-Images-metadata-processing
Added XMP read/write support.
Covered with integration tests.

Closes #1453
Closes #1450
Closes #1448
Closes #1447
Closes #1237

gabrieldagama

Hi @sivaschenko, thanks for the pull request, it looks great! Please follow some comments below.

gabrieldagama · 2020-06-24T08:51:03Z

MediaGalleryMetadata/Model/Reader/File.php

+        if (!$fileHandle) {
+            throw new LocalizedException(__('Cannot open file'));
+        }
+
+        $data = $this->read($fileHandle, 2);
+
+        if ($data != self::MARKER_IMAGE_PREFIX . self::MARKER_IMAGE_FILE_START) {
+            fclose($fileHandle);
+            throw new LocalizedException(__('Not an image'));
+        }
+
+        $data = $this->read($fileHandle, 2);
+
+        if ($data[0] != self::MARKER_IMAGE_PREFIX) {
+            fclose($fileHandle);
+            throw new LocalizedException(__('File is corrupted'));
+        }


Can we add these validations to a private method?

gabrieldagama · 2020-06-24T08:52:34Z

MediaGalleryMetadata/Model/Reader/File.php

+        while (($data[1] != self::MARKER_IMAGE_END) && (!feof($fileHandle))) {
+            if ((ord($data[1]) < 0xD0) || (ord($data[1]) > 0xD7)) {


I think it worth to wrap these conditions on methods to make the behavior explicit, especially because we are dealing with functions and operations that are not used that often (by us at least).

gabrieldagama · 2020-06-24T09:05:15Z

MediaGalleryMetadata/Model/Reader/File.php

+                $sizestr = $this->read($fileHandle, 2);
+
+                $decodedsize = unpack("nsize", $sizestr);
+
+                $segmentDataStartPosition = ftell($fileHandle);
+
+                $segmentData = $this->read($fileHandle, $decodedsize['size'] - 2);
+
+                $segments[] = $this->segmentFactory->create([
+                    'name' => $this->segmentNames->getSegmentName(ord($data[1])),
+                    'dataStart' => $segmentDataStartPosition,
+                    'data' => $segmentData,


Can we wrap this to a private function? Something like createSegment($segmentName, $fileHandle)

gabrieldagama · 2020-06-24T09:07:29Z

MediaGalleryMetadata/Model/Reader/File.php

+                $compressedImage = '';
+                do {
+                    $compressedImage .= $this->read($fileHandle, 1048576);
+                } while (!feof($fileHandle));
+
+                $endOfImageMarkerPosition = strpos($compressedImage, self::MARKER_IMAGE_PREFIX . self::MARKER_IMAGE_END);
+                $compressedImage = substr($compressedImage, 0, $endOfImageMarkerPosition);
+
+                fclose($fileHandle);
+
+                return $this->fileFactory->create([
+                    'path' => $path,
+                    'compressedImage' => $compressedImage,
+                    'segments' => $segments,
+                ]);


Similar thing here, can we wrap this on private function? Maybe also add constant for values like 1048576 and 2, that are used on read method, mostly to explain what they are.

gabrieldagama · 2020-06-24T09:10:21Z

MediaGalleryMetadata/Model/Writer/File.php

+    public function execute(FileDataObject $file): void
+    {
+        foreach ($file->getSegments() as $segment) {
+            if (strlen($segment->getData()) > 0xfffd) {


Do we need to use hexadecimal here, wouldn't decimal work as well?

hexidecimal looks much more cool

gabrieldagama · 2020-06-24T09:14:04Z

MediaGalleryMetadata/Model/SegmentNames.php

+ */
+class SegmentNames
+{
+    private const SEGMENT_TYPE_TO_NAME = [


Do we need to use hexadecimal values here? I guess it makes it harder to understand and when debugging the application.

gabrieldagama · 2020-06-24T09:15:33Z

MediaGalleryMetadata/Model/Writer/File.php

+        fwrite($fileHandler, self::MARKER_IMAGE_PREFIX . self::MARKER_IMAGE_FILE_START);
+        foreach ($file->getSegments() as $segment) {
+            fwrite($fileHandler, self::MARKER_IMAGE_PREFIX . $this->segmentNames->getSegmentType($segment->getName()));
+            fwrite($fileHandler, pack("n", strlen($segment->getData()) + 2));


The same thing with the 2 here, having it as constant would make it easier to understand and change in the future (if needed).

konarshankar07 · 2020-06-24T11:25:14Z

MediaGalleryMetadata/Model/Writer/File.php

+            throw new LocalizedException(__('Could not open file.'));
+        }
+
+        fwrite($fileHandler, self::MARKER_IMAGE_PREFIX . self::MARKER_IMAGE_FILE_START);


For fwrite function, we can use Magento\Framework\Filesystem\DriverInterface::fileWrite()

yes we can use the driver for all file operations, thanks

konarshankar07 · 2020-06-24T11:29:30Z

MediaGalleryMetadata/Model/Reader/File.php

+        $data = $this->read($fileHandle, 2);
+
+        if ($data != self::MARKER_IMAGE_PREFIX . self::MARKER_IMAGE_FILE_START) {
+            fclose($fileHandle);


For fclose function, we can use Magento\Framework\Filesystem\DriverInterface::fileClose()

sivaschenko · 2020-06-24T18:57:54Z

@gabrieldagama @konarshankar07 thanks for your suggestions! I have added the approximate structure of the module, please take a look

sivaschenko · 2020-06-29T17:25:08Z

@magento run all tests

gabrieldagama · 2020-06-30T08:44:47Z

@magento run all tests

sivaschenko · 2020-07-01T17:44:12Z

@magento run all tests

gabrieldagama

HI @sivaschenko thanks for the PR it looks awesome, well done. Please follow some suggestions below.

gabrieldagama · 2020-07-02T10:08:29Z

MediaGalleryMetadata/Model/ExtractMetadata.php

+        foreach ($this->readerPool->get() as $reader) {
+            $data = $reader->execute($path);
+            $title = $data->getTitle() ?? $title;
+            $description = $data->getDescription() ?? $description;
+            $keywords = array_unique(array_merge($keywords, $data->getKeywords()));
+            if (!empty($title) && !empty($description) && !empty($keywords)) {
+                break;
+            }
+        }


I personally find this hard to understand, we are executing the reader from all formats on one file, each of those will return a MetadataInterface, after extracting the values and validating them we create a new MetadataInterface to return.

I would prefer to have a class that receives the path and returns the correct ExtractMetadataInterface for that file, also I think readerPool is misleading, I think the name extractMetadataPool would be more straight forward, I got confused and thought the readerPool was a pool of FileReader classes.

Returning the correct ExtractMetadataInterface will:

Expose internal implementation

Make the client implementation more complex

Will not allow using multiple ExtractMetadataInterface implementation to process a single image from different angles)

Aggree with extractMetadataPool. Btw, as for the naming, do you think we should use extractMetadata term or getMetadata term for all classes?

I think extractMetadata makes sense, we are actually doing more than just getting it, I mean, usually gets are simple functions, and here we are a more complex functionality.

gabrieldagama · 2020-07-02T10:11:42Z

MediaGalleryMetadata/Model/Gif/ExtractMetadata.php

+        if (!$this->fileReader->isApplicable($path)) {
+            return $this->metadataFactory->create([
+                'title' => $title,
+                'description' => $description,
+                'keywords' => $keywords
+            ]);
+        }


Instead of returning an empty MetaDataInterface wouldn't be better to throw an exception and on your client, you just handle that and continue?

I don't think there are any differences for us if the metadata fields are empty or metadata is missing. To have only one flow of processing this situation I decided to always return a MetadataInterface. The MetadataInterface contains the information that the reader was able to retrieve.

Also, this approach provides more flexibility to extensions using plugins

gabrieldagama · 2020-07-02T10:30:04Z

MediaGalleryMetadata/Model/Gif/Segment/XmpReader.php

+    {
+        $xmp = substr($segment->getData(), 13);
+
+        if (substr($xmp, -257, 3) !== "\x01\xFF\xFE" || substr($xmp, -4) !== "\x03\x02\x01\x00") {


Maybe worth adding a comment why this condition means corrupt data

I will work on improving exception messages and exception handling in general

Introduced constants with comments

gabrieldagama · 2020-07-02T10:33:27Z

MediaGalleryMetadata/Model/Png/ExtractMetadata.php

+    /**
+     * @inheritdoc
+     */
+    public function execute(string $path): MetadataInterface


All the ExtractMetadata classes seem to be doing exactly the same thing, to avoid code duplication wouldn't be better to use virtual type or have a builder for this?

I am still not exactly sure the code for each extract metadata implementation is going to be exactly the same, but yes that's something I was also thinking about, probably worth going for it, thanks!

Added virtual types

sivaschenko · 2020-07-02T11:31:07Z

@magento run all tests

sivaschenko · 2020-07-02T11:54:19Z

https://m2build-comm-2.devops.magento.com/job/MTS-API-Tests/13082/

sivaschenko · 2020-07-02T16:05:38Z

@magento run all tests

sivaschenko · 2020-07-02T17:48:21Z

https://m2build-comm.devops.magento.com/job/MTS-API-Tests/13030/

sivaschenko · 2020-07-02T17:52:57Z

@magento run all tests

sivaschenko · 2020-07-02T17:53:27Z

https://m2build-comm.devops.magento.com/job/MTS-API-Tests/13037/

sivaschenko · 2020-07-03T09:35:07Z

@magento run Integration Tests

sivaschenko · 2020-07-03T12:22:20Z

http://10.234.235.163:8080/job/Integration-Tests/1908

sivaschenko · 2020-07-03T16:54:56Z

@magento run Integration Tests

ghost · 2020-07-04T18:13:14Z

Hi @sivaschenko, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento#1183: Initial prototype implementation

83acf4d

gabrieldagama self-assigned this Jun 23, 2020

m2-community-project bot added the Progress: review label Jun 23, 2020

gabrieldagama reviewed Jun 24, 2020

View reviewed changes

konarshankar07 reviewed Jun 24, 2020

View reviewed changes

m2-community-project bot assigned sivaschenko Jun 24, 2020

sivaschenko added 2 commits June 24, 2020 17:24

Added filesystem driver usage and fixed writer test

af3cc4d

Added module skeleton

ffae591

Added XMP reader and writer

837f3c2

sivaschenko added 2 commits June 29, 2020 18:28

removed unnecessary filter

dad2563

Fixed references and comments

7e865b5

sivaschenko changed the title ~~[WIP] Image metadata parsing prototype~~ Introduced MediaGalleryMetadata services with XMP support Jun 29, 2020

sivaschenko added 2 commits June 30, 2020 13:30

Added PNG support

9a5ccea

Added PNG write and GIF read support

4885cf1

sivaschenko marked this pull request as ready for review July 1, 2020 17:44

sivaschenko requested review from Nazar65 and gabrieldagama July 1, 2020 17:44

gabrieldagama reviewed Jul 2, 2020

View reviewed changes

Merge branch '2.0-develop' into metadata-prototype

38bc7b8

Fixed static tests and review notes

d38cf0f

gabrieldagama previously approved these changes Jul 2, 2020

View reviewed changes

Fixed static tests

b994614

sivaschenko dismissed gabrieldagama’s stale review via b994614 July 2, 2020 17:52

Nazar65 approved these changes Jul 2, 2020

View reviewed changes

Merge branch '2.0-develop' into metadata-prototype

68e7800

sivaschenko merged commit 6d29819 into magento:2.0-develop Jul 4, 2020

m2-community-project bot removed the Progress: review label Jul 4, 2020

		while (($data[1] != self::MARKER_IMAGE_END) && (!feof($fileHandle))) {
		if ((ord($data[1]) < 0xD0) \|\| (ord($data[1]) > 0xD7)) {

Introduced MediaGalleryMetadata services with XMP support #1514

Introduced MediaGalleryMetadata services with XMP support #1514

Conversation

sivaschenko commented Jun 23, 2020 • edited Loading

gabrieldagama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivaschenko commented Jun 24, 2020

sivaschenko commented Jun 29, 2020

gabrieldagama commented Jun 30, 2020

sivaschenko commented Jul 1, 2020

gabrieldagama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivaschenko commented Jul 2, 2020

sivaschenko commented Jul 2, 2020

sivaschenko commented Jul 2, 2020

sivaschenko commented Jul 2, 2020

sivaschenko commented Jul 2, 2020

sivaschenko commented Jul 2, 2020

sivaschenko commented Jul 3, 2020

sivaschenko commented Jul 3, 2020

sivaschenko commented Jul 3, 2020

ghost commented Jul 4, 2020

sivaschenko commented Jun 23, 2020 •

edited

Loading