Skip to content

Help needed: Support for XMP metadata in MP4 files#1684

Merged
dirkhh merged 2 commits intosubsurface:masterfrom
bstoeger:metadata2
Sep 25, 2018
Merged

Help needed: Support for XMP metadata in MP4 files#1684
dirkhh merged 2 commits intosubsurface:masterfrom
bstoeger:metadata2

Conversation

@bstoeger
Copy link
Copy Markdown
Collaborator

@bstoeger bstoeger commented Sep 15, 2018 •
edited
Loading

Describe the pull request:

  • Bug fix
  • Functional change
  • New feature
  • Code cleanup
  • Build system change
  • Documentation change
  • Language translation

Pull request long description:

As @willemferguson noted, some video-manipulation software updates the XMP-metadata, but not the native metadata of MP4s. Therefore, we'll have to support XMP-metadata. This is a first rudimentary attempt.

It follows the bad practice of implementing a pseudo-XML parser instead of using a real, robust XML parser. I was too lazy to get Qt's XML parser to work. Moreover, Qt's parser is rumored to be slow and therefore it might be a problem if directories with thousands of pictures are imported (though without measurement this is purely speculation).

In any case it turns out the XMP metadata is not standardized at all. Unfortunately I don't possess videos with such metadata. Therefore I would need help:

If you have such files, please give this a try and if it doesn't work, send me the appropriate metadata-block. It usually is at the end of the file and starts with something like <?xpacket begin='' id='W5M0MpCehiHzreSzNTczkc9d'?>. The few bytes before the start probably help too.

Thanks.

I will then try to add support for the other file types [FLV, AVI, JPG].

Changes made:

  1. Add XMP parser.
  2. Connect XMP parser to MP4 parser.

Related issues:

Additional information:

Release note:

Done.

Documentation change:

Perhaps? Or too detailed?

Mentions:

@janmulder
Copy link
Copy Markdown
Collaborator

janmulder commented Sep 15, 2018 •
edited
Loading

Maybe a stupid remark, but isn't there some open source library that does 95% of the work you now want to implement? We use all kinds of external sources in the project, so adding one should not be a fundamental issue.

@bstoeger
Copy link
Copy Markdown
Collaborator Author

bstoeger commented Sep 15, 2018

I'm sure that such a thing exists. But for me it is easier to write a simple parser than to include an external library in the build system (Travis, Packages, etc). Moreover, we have more control [e.g. prioritize XMP over native, etc]. On the flip side, an external library will be more complete. Some problems will remain: which of the numerous dates that one can find in a video file do we use..?

I certainly wouldn't be offended with someone replacing either the XMP-parser with Qt's XML-parser or the whole metadata parsing by an external library. Personally, I just feel too lazy to even start checking the options.

@bstoeger
Copy link
Copy Markdown
Collaborator Author

bstoeger commented Sep 16, 2018

Some cosmetic changes. Notably, don't use the ugly "blank-out uuid" hack.

@bstoeger bstoeger force-pushed the metadata2 branch 5 times, most recently from dd4fb4f to 172be47 Compare September 18, 2018 10:45
@neolit123
Copy link
Copy Markdown
Member

neolit123 commented Sep 20, 2018

@bstoeger i must admit i concur with @janmulder 's observation.

re-inventing the wheel is often an excellent brain exercise, but maintenance is an issue here.
you have the time for the implementation and you can maintain the code, but how about others?

who has the time to review such a change and co-maintain it?

It follows the bad practice of implementing a pseudo-XML parser instead of using a real, robust XML parser. I was too lazy to get Qt's XML parser to work. Moreover, Qt's parser is rumored to be slow and therefore it might be a problem if directories with thousands of pictures are imported (though without measurement this is purely speculation).

i do not like the idea of a custom parser, for the sake of not using Qt's XML parser.

FLV

if this is flash video, no need. the format is as dead as flash.
https://en.wikipedia.org/wiki/Flash_Video

JPG

is this https://en.wikipedia.org/wiki/Motion_JPEG ?
unless a fair amount of cameras have this, my initial suggestion would be to not bother.

@bstoeger
Copy link
Copy Markdown
Collaborator Author

bstoeger commented Sep 20, 2018

re-inventing the wheel is often an excellent brain exercise, but maintenance is an issue here.
you have the time for the implementation and you can maintain the code, but how about others?

I don't know. I tried to use an easy-to-follow imperative C-style approach.

A user (@willemferguson) reported a problem and I tried to fix it in the easiest possible way for me. Though I haven't heard of a confirmation that it works. If someone wants to replace the XML-parser with Qt's implementation that's fine. Personally, I just have too many other open issues. If the general feeling is that this brings more pain than gain - let's dump it. Also fine.

i do not like the idea of a custom parser, for the sake of not using Qt's XML parser.

Using Qt's XML parser was the first plan, but after failing to add it to the project and seeing the interface I decided that it's easier for me to write my own. XML is annoying but not rocket science after all.

if this is flash video, no need. the format is as dead as flash.

Note that this is not support of FLV, but support of embedded XMP data. We already have an FLV parser. :) Feel free to remove.

is this https://en.wikipedia.org/wiki/Motion_JPEG ?

No. I'm talking about XMP in standard JPEG still images. That's the whole point of XMP: Have a "standard" (haha - in 10 minutes I found two different kinds of embedded creation dates) metadata format for all common media formats.

@bstoeger bstoeger force-pushed the metadata2 branch 2 times, most recently from 1d500d2 to c176a36 Compare September 20, 2018 11:43
@dirkhh
Copy link
Copy Markdown
Collaborator

dirkhh commented Sep 20, 2018

So here's what I don't understand. We already use libxml2. What's wrong with using that to parse XML here? IOW, we already have an XML parser. We need neither Qt's XML parser (which we didn't use in the first place because the XML parsing was implemented when we were a C application using gtk) nor another hand-crafted XML parser...

@bstoeger
Copy link
Copy Markdown
Collaborator Author

bstoeger commented Sep 20, 2018

What's wrong with using that to parse XML here?

Nothing. I simply wasn't aware of it. :)

@bstoeger
Copy link
Copy Markdown
Collaborator Author

bstoeger commented Sep 20, 2018 •
edited
Loading

What's the point of the URL argument to xmlReadMemory? Apparently the filename is passed in, but why would the XML parser have to know..?

@bstoeger bstoeger force-pushed the metadata2 branch 4 times, most recently from 3066746 to af6726b Compare September 21, 2018 05:52
@bstoeger
Copy link
Copy Markdown
Collaborator Author

bstoeger commented Sep 21, 2018

Huh?
Travis fails with:

/home/travis/build/Subsurface-divelog/subsurface/core/xmp_parser.cpp: In function ‘timestamp_t extract_timestamp(const xmlNode*)’:
/home/travis/build/Subsurface-divelog/subsurface/core/xmp_parser.cpp:92:27: error: invalid conversion from ‘const xmlNode* {aka const _xmlNode*}’ to ‘xmlNodePtr {aka _xmlNode*}’ [-fpermissive]
   if (!xmlIsBlankNode(node) && stack.size() >= 2) {
                           ^
In file included from /home/travis/build/Subsurface-divelog/subsurface/core/dive.h:17:0,
                 from /home/travis/build/Subsurface-divelog/subsurface/core/xmp_parser.cpp:2:
/usr/include/libxml2/libxml/tree.h:922:3: error:   initializing argument 1 of ‘int xmlIsBlankNode(xmlNodePtr)’ [-fpermissive]
   xmlIsBlankNode  (xmlNodePtr node);

whereas the docs say:

int	xmlIsBlankNode			(const xmlNode * node)

Are we compiling against an outdated version?

Btw: the libxml2 documentation/reference is not very good.

@bstoeger
Copy link
Copy Markdown
Collaborator Author

bstoeger commented Sep 21, 2018

Update: now using libxml2, removed all C++ constructs, such as std::vector (there's still nullptr, but I hope that doesn't offend anyone).

Still no confirmation that this works with real data apart from my synthetic dummy-videos. Such data would also be necessary for tests. :(

@bstoeger bstoeger force-pushed the metadata2 branch 3 times, most recently from e14c6d2 to 806f721 Compare September 25, 2018 16:23
@bstoeger
Copy link
Copy Markdown
Collaborator Author

bstoeger commented Sep 25, 2018

Ping...?

This now uses libxml2 (dislike the interface and documentation, but what can you do) instead of a hand-crafted parser. This seems very low risk. I tested it on 2 synthetic examples and ran it on a directory of ~100 videos. No issues. No confirmation on real-world data though (@willemferguson?).

Code is not nice, but I see no point in polishing it until we try to extract more usable data from the XMP block [e.g. GPS?]. But here too, working on that makes little sense until we get access to real-world examples.

If there is no interest in this feature - let's dump it.

dirkhh
dirkhh approved these changes Sep 25, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dirkhh dirkhh left a comment

Choose a reason for hiding this comment

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

A couple of small comments, but I think this looks good.

Comment thread core/xmp_parser.cpp
// Parse content, if not blank node. Content can only be at the second level,
// since it is always contained in a tag.
// TODO: We have to cast node to pointer to non-const, since we're supporting
// old libxml2 versions, where xmlIsBlankNode takes such a pointer. Remove
Copy link
Copy Markdown
Collaborator

@dirkhh dirkhh Sep 25, 2018

Choose a reason for hiding this comment

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

how old? we build libxml2 on some platforms from source, anyway. But in order to understand how realistic it is to say "never mind", I'd need to know how old (or better, "how new") libxml2 has to be...

Copy link
Copy Markdown
Collaborator Author

@bstoeger bstoeger Sep 25, 2018

Choose a reason for hiding this comment

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

I don't know - one of the Travis builds failed for this reason - but I don't remember which. I will push a private branch to find out.

Copy link
Copy Markdown
Collaborator Author

@bstoeger bstoeger Sep 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's the linux and linux2 builds that fail. See https://travis-ci.org/Subsurface-divelog/subsurface/jobs/433133627

[ 48%] Building CXX object core/CMakeFiles/subsurface_corelib.dir/xmp_parser.cpp.o
/home/travis/build/Subsurface-divelog/subsurface/core/xmp_parser.cpp: In function ‘timestamp_t extract_timestamp(const xmlNode*)’:
/home/travis/build/Subsurface-divelog/subsurface/core/xmp_parser.cpp:95:27: error: invalid conversion from ‘const xmlNode* {aka const _xmlNode*}’ to ‘xmlNodePtr {aka _xmlNode*}’ [-fpermissive]
   if (!xmlIsBlankNode(node) && stack_depth >= 2) {
                           ^
In file included from /usr/include/libxml2/libxml/parser.h:16:0,
                 from /home/travis/build/Subsurface-divelog/subsurface/core/xmp_parser.cpp:4:
/usr/include/libxml2/libxml/tree.h:922:3: error:   initializing argument 1 of ‘int xmlIsBlankNode(xmlNodePtr)’ [-fpermissive]
   xmlIsBlankNode  (xmlNodePtr node);
   ^
make[2]: *** [core/CMakeFiles/subsurface_corelib.dir/xmp_parser.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....

Apparently failing version:
Setting up libxml2-dev:amd64 (2.9.1+dfsg1-3ubuntu4.13)

Windows works, which is:
libxml-2.0, version 2.9.4

I guess keeping the cast is just the simple thing to do...

Edit: qt55 also works, which is 2.9.3. So the first version without the issue is 2.9.2 or 2.9.3.

Copy link
Copy Markdown
Collaborator

@dirkhh dirkhh Sep 25, 2018

Choose a reason for hiding this comment

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

Well, but that means that as of 16.04 (the oldest we actually support), we should have the right version: https://packages.ubuntu.com/search?keywords=libxml2-dev

I'm fine with keeping the cast. It's annoying that Travis still doesn't offer an out of the box 16.04 or newer.

Comment thread core/xmp_parser.cpp
// having to unwind the call-stack. We only recurse to a fixed depth,
// since the data we are interested in are at a shallow depth.
// This can be increased on demand.
static const int max_recursion_depth = 16;
Copy link
Copy Markdown
Collaborator

@dirkhh dirkhh Sep 25, 2018

Choose a reason for hiding this comment

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

interesting choice. I assume that made it easier? Faster? ??? i.e. what drove the decision to do this with a fixed stack?
(I'm not saying this is wrong, I just want to understand it better)

Copy link
Copy Markdown
Collaborator Author

@bstoeger bstoeger Sep 25, 2018

Choose a reason for hiding this comment

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

i.e. what drove the decision to do this with a fixed stack?

Laziness. Always suspect laziness first. :-P
It means that we can simple return once we have the datum that we're interested in and don't have to check at higher levels if the lower levels found what they're after.

The interface will have to be adapted anyway once we extract more than one datum, so my reasoning is that it's not worth doing this correctly now.

Copy link
Copy Markdown
Collaborator Author

@bstoeger bstoeger Sep 25, 2018

Choose a reason for hiding this comment

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

In case you ask why it's fixed depth: at first I had an automatically growing std::vector<>, but then changed it to a C-array, because I figured it would be easier to stomach. :)

Comment thread core/xmp_parser.cpp Outdated
@@ -0,0 +1,137 @@
#include "xmp_parser.h"
#include "subsurface-string.h"
#include "dive.h" // TODO: monster-include file only for utc_mktime.
Copy link
Copy Markdown
Collaborator

@dirkhh dirkhh Sep 25, 2018

Choose a reason for hiding this comment

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

hrmpf - just copy the one element that you need? that seems overkill.

Copy link
Copy Markdown
Collaborator Author

@bstoeger bstoeger Sep 25, 2018

Choose a reason for hiding this comment

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

Done.

@bstoeger
Metadata: rudimentary support for XMP metadataa in MP4-based videos
429c054 
XMP is a media-metadata standard based on XML which may be used
across a variety of media formats. Some video-processing software
writes XMP data without updating the native metadata fields.
Therefore, we should aim at reading XMP metadata and give priority
of XMP data over native fields.

Pros:
	- Support for *all* common media formats.
Cons:
	- XML (complex, verbose, chaotic).
	- Does not even come close to fulfilling its promise of being
	  well defined (see below).

Implement a simple XMP-parser using libxml2. Connect the XMP-parser to
the existing Quicktime/MP4 parser.

First problem encountered: According to the spec, XMP data supposed
to be put in the 'XMP_' atom. But for example exiftools instead
writes an 'uuid' atom with a special 16-byte uid. Implement both,
more options will probably follow.

Second problem: two versions of recording the creation date were found
  1) The content of a <exif:DateTimeOriginal> tag.
  2) The xmp::CreateDate attribute of a <rdf:Description> tag.

Here too, more versions are expected to surface and will have
to be supported in due course (with an obvious priority problem).

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>
@bstoeger
CHANGELOG.md update
f81d66b 
Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>
@dirkhh dirkhh merged commit b19fe27 into subsurface:master Sep 25, 2018
@willemferguson
Copy link
Copy Markdown
Contributor

willemferguson commented Sep 26, 2018 via email

@willemferguson
Copy link
Copy Markdown
Contributor

willemferguson commented Sep 26, 2018 via email

@bstoeger bstoeger deleted the metadata2 branch April 7, 2020 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dirkhh dirkhh dirkhh approved these changes

Assignees

No one assigned

Labels

dive-video rfc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bstoeger @janmulder @neolit123 @dirkhh @willemferguson