Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Parse Frameright app generated region metafields #16

Merged
merged 2 commits into from
Oct 9, 2023
Merged

Parse Frameright app generated region metafields #16

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e810a0
Parse Frameright app generated region metafields
klaari Oct 9, 2023
3752981
Do not return regionDefinitionId and regionName by default
klaari Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Image.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ public function getIDCMetadata(
'radius' => $region->rbRx,

'vertices' => [],

'regionDefinitionId' => $region->regionDefinitionId,
'regionName' => $region->regionName,
Comment on lines +120 to +122
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to return these values only when $essentialOnly is false. Would this work for you @klaari? Thoughts @ilu?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, works fine with me. I can do the update if we all agree?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me @AurelienLourot

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klaari yes you can do the update, thanks a lot

];

if ($region->rbVertices) {
Expand Down
6 changes: 6 additions & 0 deletions src/Metadata/Xmp.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ class Xmp
*/
const PHOTO_MECHANIC_NS = "http://ns.camerabits.com/photomechanic/1.0/";

/**
*
*/
const FRAMERIGHT_IDC_NS = 'http://ns.frameright.io/idc/1.0/';

/**
* @var \DomDocument
*/
Expand Down Expand Up @@ -80,6 +85,7 @@ class Xmp
'xmpRights' => self::XMP_RIGHTS_NS,
'Iptc4xmpCore' => self::IPTC4_XMP_CORE_NS,
'Iptc4xmpExt' => self::IPTC4_XMP_EXT_NS,
'FramerightIdc' => self::FRAMERIGHT_IDC_NS,
'photomechanic' => self::PHOTO_MECHANIC_NS
];

Expand Down
27 changes: 27 additions & 0 deletions src/Metadata/Xmp/ImageRegion.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,21 @@ class ImageRegion
*/
public $rbVertices;


/**
* Region's definition ID from the Frameright service.
*
* @var string|null
*/
public $regionDefinitionId;

/**
* Region's definition name from the Frameright service.
*
* @var string|null
*/
public $regionName;

/**
* Initialize members if a node is provided.
*
Expand Down Expand Up @@ -111,6 +126,18 @@ public function __construct($xpath = null, $node = null)
$node
);

$this->regionDefinitionId = self::getNodeValue(
$xpath,
'FramerightIdc:RegionDefinitionId',
$node
);

$this->regionName = self::getNodeValue(
$xpath,
'FramerightIdc:RegionName',
$node
);

$xpathToRb = 'Iptc4xmpExt:RegionBoundary';

foreach ([
Expand Down
Binary file added tests/Fixtures/frameright.jpg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions tests/Format/AbstractImageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ public function testGetIDCMetadata()
$inputRectangleRegion->rbXY = new Point(0.31, 0.18);
$inputRectangleRegion->rbH = 0.385;
$inputRectangleRegion->rbW = 0.127;
$inputRectangleRegion->regionDefinitionId = 'rectangleregiondefid';
$inputRectangleRegion->regionName = 'rectangleregionname';

$inputCircleRegion = new ImageRegion();
$inputCircleRegion->id = 'persltr3';
Expand All @@ -97,6 +99,8 @@ public function testGetIDCMetadata()
$inputCircleRegion->rbUnit = 'relative';
$inputCircleRegion->rbXY = new Point(0.59, 0.426);
$inputCircleRegion->rbRx = 0.068;
$inputCircleRegion->regionDefinitionId = 'circleregiondefid';
$inputCircleRegion->regionName = 'circleregionname';

$inputPolygonRegion = new ImageRegion();
$inputPolygonRegion->id = 'persltr1';
Expand All @@ -119,6 +123,8 @@ public function testGetIDCMetadata()
new Point(0.148, 0.041),
new Point(0.375, 0.863),
];
$inputPolygonRegion->regionDefinitionId = 'polygonregiondefid';
$inputPolygonRegion->regionName = 'polygonregionname';

$xmp->expects($this->atLeastOnce())->method('getImageRegions')->with(
ShapeFilter::ANY,
Expand Down Expand Up @@ -152,6 +158,8 @@ public function testGetIDCMetadata()
'height' => 0.385,
'radius' => null,
'vertices' => [],
'regionDefinitionId' => 'rectangleregiondefid',
'regionName' => 'rectangleregionname',
];

$expectedCircleRegion = [
Expand All @@ -177,6 +185,8 @@ public function testGetIDCMetadata()
'height' => null,
'radius' => 0.068,
'vertices' => [],
'regionDefinitionId' => 'circleregiondefid',
'regionName' => 'circleregionname',
];

$expectedPolygonRegion = [
Expand Down Expand Up @@ -215,6 +225,8 @@ public function testGetIDCMetadata()
'y' => 0.863,
],
],
'regionDefinitionId' => 'polygonregiondefid',
'regionName' => 'polygonregionname',
];

$this->assertEquals([
Expand Down
33 changes: 33 additions & 0 deletions tests/Metadata/XmpTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,39 @@ public function testGetImageRegions()
], $xmp->getImageRegions(ShapeFilter::RECTANGLE, RoleFilter::CROP));
}

/**
* @covers ::getImageRegions
*/
public function testGetImageRegionFromFramerightImage()
{
$jpeg = JPEG::fromFile(
__DIR__ . '/../Fixtures/frameright.jpg');


$xmp = $jpeg->getXmp();

$expectedRegion = new ImageRegion();
$expectedRegion->regionDefinitionId = 'definition-7a54f275-6872-435e-befc-b52d97653a28';
$expectedRegion->regionName = '4:3 Horizontal';
$expectedRegion->id = 'crop-e789b6b8-ee15-45c0-a0c5-3ad38858db14';
$expectedRegion->names = null;
Comment on lines +385 to +388
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilu I'm surprised the IDC names bag is empty, instead of containing the same value as what we put in FramerightIdc:RegionName. Is this a bug in https://frameright.app ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test image I created with the https://frameright.app had the region name only in the FramerightIdc:RegionName
This is the full xmpmeta from the test file:

<x:xmpmeta
    xmlns:x="adobe:ns:meta/" x:xmptk="Frameright XMP Toolkit 2.0.0">
    <rdf:RDF
        xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
        <rdf:Description
            xmlns:Iptc4xmpExt="http://iptc.org/std/Iptc4xmpExt/2008-02-29/"
            xmlns:FramerightIdc="http://ns.frameright.io/idc/1.0/">
            <Iptc4xmpExt:ImageRegion
                xmlns:Iptc4xmpExt="http://iptc.org/std/Iptc4xmpExt/2008-02-29/">
                <rdf:Bag>
                    <rdf:li rdf:parseType="Resource">
                        <Iptc4xmpExt:RegionBoundary rdf:parseType="Resource">
                            <Iptc4xmpExt:rbShape>rectangle</Iptc4xmpExt:rbShape>
                            <Iptc4xmpExt:rbUnit>relative</Iptc4xmpExt:rbUnit>
                            <Iptc4xmpExt:rbW>0.8890625</Iptc4xmpExt:rbW>
                            <Iptc4xmpExt:rbH>1</Iptc4xmpExt:rbH>
                            <Iptc4xmpExt:rbX>0.0375</Iptc4xmpExt:rbX>
                            <Iptc4xmpExt:rbY>0</Iptc4xmpExt:rbY>
                        </Iptc4xmpExt:RegionBoundary>
                        <Iptc4xmpExt:rId>crop-e789b6b8-ee15-45c0-a0c5-3ad38858db14</Iptc4xmpExt:rId>
                        <Iptc4xmpExt:rRole rdf:parseType="Resource">
                            <rdf:Bag>
                                <rdf:li rdf:parseType="Resource">
                                    <xmp:Identifier
                                        xmlns:xmp="http://ns.adobe.com/xap/1.0/">
                                        <rdf:Bag>
                                            <rdf:li>http://cv.iptc.org/newscodes/imageregionrole/cropping</rdf:li>
                                        </rdf:Bag>
                                    </xmp:Identifier>
                                </rdf:li>
                            </rdf:Bag>
                        </Iptc4xmpExt:rRole>
                        <FramerightIdc:RegionDefinitionId
                            xmlns:FramerightIdc="http://ns.frameright.io/idc/1.0/">definition-7a54f275-6872-435e-befc-b52d97653a28
                        </FramerightIdc:RegionDefinitionId>
                        <FramerightIdc:RegionName
                            xmlns:FramerightIdc="http://ns.frameright.io/idc/1.0/">4:3 Horizontal
                        </FramerightIdc:RegionName>
                    </rdf:li>
                </rdf:Bag>
            </Iptc4xmpExt:ImageRegion>
            <MetadataDate
                xmlns="http://ns.adobe.com/xap/1.0/">2023-10-08T06:53:22.336Z
            </MetadataDate>
            <ModifyDate
                xmlns="http://ns.adobe.com/xap/1.0/">2023-10-08T06:53:22.336Z
            </ModifyDate>
        </rdf:Description>
    </rdf:RDF>
</x:xmpmeta>```

Copy link
Member

@ilu ilu Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AurelienLourot It's more of an unimplemented feature. The FramerightIdc fields are intended for use with Frameright.app, so they have a bit different semantics than the Iptc4xmpExt:Name field. For example Iptc4xmpExt:Name needs to be unique and can have translations, both of which we would have to handle somehow on Frameright.app.

But yes, we should enable writing that field and some other fields as well in Frameright.app, but we need to figure out how to handle the ones that have conflicting semantics.

If the region has an existing Iptc4xmpExt:Name field (or any other fields), we leave them intact, and we only update the x, y, width, height, unit etc. fields and the FramerightIdc fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for clarifying. @klaari this doesn't have any impact on this PR I was just curious

$expectedRegion->types = null;
$expectedRegion->roles = [
'http://cv.iptc.org/newscodes/imageregionrole/cropping',
];
$expectedRegion->rbShape = 'rectangle';
$expectedRegion->rbUnit = 'relative';
$expectedRegion->rbXY = new Point(0.0375, 0);
$expectedRegion->rbRx = null;
$expectedRegion->rbH = '1';
$expectedRegion->rbW = '0.8890625';

$this->assertEquals([
$expectedRegion,
], $xmp->getImageRegions());

}

/**
* @param Xmp $xmp
*/
Expand Down