Expose IPTC and XMP in metadata if available #1079

oaleynik · 2017-12-28T18:33:01Z

Closes #904

oaleynik · 2017-12-28T18:34:28Z

@lovell I didn't see references to the WIP branch like mentioned in Contribution guide, so I've created PR targeting master branch. Feel free to change target branch.

coveralls · 2017-12-28T18:50:23Z

Coverage remained the same at 99.226% when pulling 61f7590 on oaleynik:feat__iptc-xmp into 358b8fe on lovell:master.

coveralls · 2017-12-28T19:02:16Z

Coverage remained the same at 99.226% when pulling 7a66294 on oaleynik:feat__iptc-xmp into 358b8fe on lovell:master.

coveralls · 2017-12-28T19:19:09Z

Coverage decreased (-77.2%) to 21.981% when pulling d82dbc0 on oaleynik:feat__iptc-xmp into 358b8fe on lovell:master.

oaleynik · 2017-12-28T19:24:49Z

Oops, that was my bad with coverage - forgot to remove .only from test.

oaleynik · 2017-12-28T19:24:57Z

Rebased and squashed.

coveralls · 2017-12-28T19:40:10Z

Coverage remained the same at 99.226% when pulling 65f3c94 on oaleynik:feat__iptc-xmp into 358b8fe on lovell:master.

lovell · 2017-12-31T09:48:57Z

Hello, this looks great, thank you. Do you think it's worth adding an assertion that inspects the first byte/word of each Buffer to ensure their contents are as-expected too?

Update: I'm happy for devDependencies to be added that can parse the metadata for use in the tests.

lovell · 2017-12-31T09:50:58Z

src/metadata.cc

@@ -81,6 +81,22 @@ class MetadataWorker : public Nan::AsyncWorker {
        memcpy(baton->icc, icc, iccLength);
        baton->iccLength = iccLength;
      }
+      // IPTC
+      if (image.get_typeof(VIPS_META_IPCT_NAME) == VIPS_TYPE_BLOB) {


@jcupitt Was the use of IPCT (vs IPTC) in the VIPS_META_IPCT_NAME naming intentional/historical?

Oh dear, no, just acronymical dyslexia, sorry. I'll fix it.

Fixed in 8.6 with jcupitt/libvips@77287a9

There's a #define so the wrong name will still work.

@jcupitt Thanks John, fast work as always.

@oaleynik This PR can use the current IPCT spelling for now. The forthcoming version of sharp will depend on libvips v8.6.1+ so I'll update to use the IPTC naming before release.

Rodger! Thanks for the quick fix @jcupitt ;)

lovell · 2017-12-31T09:54:42Z

test/fixtures/index.js

@@ -57,6 +57,7 @@ module.exports = {

  inputJpg: getPath('2569067123_aca715a2ee_o.jpg'), // http://www.flickr.com/photos/grizdave/2569067123/
  inputJpgWithExif: getPath('Landscape_8.jpg'), // https://github.com/recurser/exif-orientation-examples/blob/master/Landscape_8.jpg
+  inputJpgWithIptcAndXmp: getPath('Landscape_9.jpg'), // https://unsplash.com/photos/RWAIyGmgHTQ


Is there a smaller test image we could use? (Perhaps resize this one but maintain metadata?)

Oops, sorry. I will definitely resize it.

oaleynik · 2017-12-31T17:53:55Z

Hello, this looks great, thank you. Do you think it's worth adding an assertion that inspects the first byte/word of each Buffer to ensure their contents are as-expected too?
Update: I'm happy for devDependencies to be added that can parse the metadata for use in the tests.

@lovell I wanted to add some kind of IPTC parser, but I didn't find good implementation for nodejs, which is at least maintained. For use in my main job project I've ended up implementing the custom one (I need to be able to use streams). Not sure if I will be releasing that as independent npm package, but I will definitely think about adding here something like this https://github.com/devongovett/exif-reader/blob/master/index.js#L4

coveralls · 2017-12-31T20:38:07Z

Coverage remained the same at 99.226% when pulling 333765c on oaleynik:feat__iptc-xmp into 358b8fe on lovell:master.

lovell · 2018-01-07T12:25:26Z

test/unit/metadata.js

+      // IPTC
+      assert.strictEqual('object', typeof metadata.iptc);
+      assert.strictEqual(true, metadata.iptc instanceof Buffer);
+      // XMP


Given there's no well-maintained IPTC parser, perhaps we could add an assertion on the Buffer length?

This can work for now. Thanks for the suggestion @lovell! I will add it.

lovell · 2018-01-07T12:27:12Z

test/unit/metadata.js

+      assert.strictEqual(true, metadata.iptc instanceof Buffer);
+      // XMP
+      assert.strictEqual('object', typeof metadata.xmp);
+      assert.strictEqual(true, metadata.xmp instanceof Buffer);


XMP is XML-ish so perhaps an assertion that the Buffer starts with <?xpacket will be enough here?

I've went through specs and it looks like <?xpacket will not be enough... I may be wrong though.

It seems that byte length of both xmp and iptc property returned are equal. And content looks similar. Seems like we don't get appropriate buffers.

oaleynik · 2018-01-09T21:31:43Z

@lovell I've added tests for buffer length for both xmp and iptc properties. Also for iptc I have test for first bytes which should equal to Photoshop word. These tests should be failing now - looks like buffers which we get are wrong.

lovell

Hello, the tests are failing as the ICC blob is erroneously being used - I've added a couple of comments inline.

lovell · 2018-01-10T15:00:19Z

src/metadata.cc

+      // IPTC
+      if (image.get_typeof(VIPS_META_IPCT_NAME) == VIPS_TYPE_BLOB) {
+        size_t iptcLength;
+        void const *iptc = image.get_blob(VIPS_META_ICC_NAME, &iptcLength);


- void const *iptc = image.get_blob(VIPS_META_ICC_NAME, &iptcLength); + void const *iptc = image.get_blob(VIPS_META_IPTC_NAME, &iptcLength);

@lovell fixed. Tests passing locally. Waiting for CI

lovell · 2018-01-10T15:01:04Z

src/metadata.cc

+      // XMP
+      if (image.get_typeof(VIPS_META_XMP_NAME) == VIPS_TYPE_BLOB) {
+        size_t xmpLength;
+        void const *xmp = image.get_blob(VIPS_META_ICC_NAME, &xmpLength);


- void const *xmp = image.get_blob(VIPS_META_ICC_NAME, &xmpLength); + void const *xmp = image.get_blob(VIPS_META_XMP_NAME, &xmpLength);

Uh, silly me.

@lovell fixed. Tests passing locally. Waiting for CI

coveralls · 2018-01-10T18:48:04Z

Coverage remained the same at 99.226% when pulling 47ff8c0 on oaleynik:feat__iptc-xmp into 8afcb16 on lovell:master.

oaleynik · 2018-01-10T19:24:11Z

Added one more test for xmp:

assert.strictEqual(metadata.xmp.indexOf(Buffer.from('http://ns.adobe.com/xap/1.0')), 0);

oaleynik · 2018-01-10T19:48:09Z

@lovell strange failure in AppVayor

coveralls · 2018-01-10T19:53:50Z

Coverage remained the same at 99.226% when pulling 99c49f3 on oaleynik:feat__iptc-xmp into 8afcb16 on lovell:master.

lovell · 2018-01-10T22:12:36Z

Brilliant, thank you Oleg, this will be in v0.19.0.

oaleynik · 2018-01-10T22:13:19Z

Woot! Thanks for helping @lovell @jcupitt!

rayshan · 2018-01-16T19:24:09Z

Thanks you guys rock! Will try later.

oaleynik changed the title ~~[WIP] Expose IPTC and XMP in metadata if available~~ Expose IPTC and XMP in metadata if available Dec 28, 2017

oaleynik force-pushed the feat__iptc-xmp branch from d82dbc0 to 65f3c94 Compare December 28, 2017 19:24

lovell reviewed Dec 31, 2017

View reviewed changes

oaleynik force-pushed the feat__iptc-xmp branch from 65f3c94 to 333765c Compare December 31, 2017 20:18

lovell reviewed Jan 7, 2018

View reviewed changes

oaleynik force-pushed the feat__iptc-xmp branch from 333765c to 5e95688 Compare January 9, 2018 21:30

lovell requested changes Jan 10, 2018

View reviewed changes

oaleynik force-pushed the feat__iptc-xmp branch from 5e95688 to 47ff8c0 Compare January 10, 2018 17:50

feat: expose iptc and xmp in metadata if available

99c49f3

oaleynik force-pushed the feat__iptc-xmp branch from 47ff8c0 to 99c49f3 Compare January 10, 2018 19:23

lovell approved these changes Jan 10, 2018

View reviewed changes

lovell merged commit c4df115 into lovell:master Jan 10, 2018

lovell added this to the v0.19.0 milestone Jan 10, 2018

lovell added a commit that referenced this pull request Jan 10, 2018

Doc refresh for #1079

d5295a2

oaleynik deleted the feat__iptc-xmp branch January 11, 2018 00:49

Expose IPTC and XMP in metadata if available #1079

Expose IPTC and XMP in metadata if available #1079

Conversation

oaleynik commented Dec 28, 2017

oaleynik commented Dec 28, 2017

coveralls commented Dec 28, 2017 • edited Loading

coveralls commented Dec 28, 2017 • edited Loading

coveralls commented Dec 28, 2017 • edited Loading

oaleynik commented Dec 28, 2017

oaleynik commented Dec 28, 2017

coveralls commented Dec 28, 2017 • edited Loading

lovell commented Dec 31, 2017 • edited Loading

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

oaleynik commented Dec 31, 2017

coveralls commented Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovell Jan 7, 2018 • edited Loading

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

oaleynik commented Jan 9, 2018

lovell 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

coveralls commented Jan 10, 2018 • edited Loading

oaleynik commented Jan 10, 2018

oaleynik commented Jan 10, 2018

coveralls commented Jan 10, 2018 • edited Loading

lovell commented Jan 10, 2018

oaleynik commented Jan 10, 2018

rayshan commented Jan 16, 2018

coveralls commented Dec 28, 2017 •

edited

Loading

coveralls commented Dec 28, 2017 •

edited

Loading

coveralls commented Dec 28, 2017 •

edited

Loading

coveralls commented Dec 28, 2017 •

edited

Loading

lovell commented Dec 31, 2017 •

edited

Loading

coveralls commented Dec 31, 2017 •

edited

Loading

lovell Jan 7, 2018 •

edited

Loading

coveralls commented Jan 10, 2018 •

edited

Loading

coveralls commented Jan 10, 2018 •

edited

Loading