Skip to content

Added support for QuickTime Metadata ItemList Tags besides the Keyed Tags#420

Open
stefanlenselink wants to merge 7 commits intodrewnoakes:mainfrom
stefanlenselink:quicktime-itemlist-metadata
Open

Added support for QuickTime Metadata ItemList Tags besides the Keyed Tags#420
stefanlenselink wants to merge 7 commits intodrewnoakes:mainfrom
stefanlenselink:quicktime-itemlist-metadata

Conversation

@stefanlenselink
Copy link
Copy Markdown

Implemented a subset of the Quicktime ItemList Tags see ExifTool. Basicly matched the already defined Tags from the Keys Tags to their ItemList Tags counter parts. Also added support the for the 'meta' node inside the UserData node.

…Tags. Also support metadata inside the UserData tag
… a best effort as we don't implement the full set.
@drewnoakes
Copy link
Copy Markdown
Owner

Thanks! This looks great. It'll be a while before I get to reviewing it, sorry. I'm very busy at work right now.

@drewnoakes
Copy link
Copy Markdown
Owner

I'm working to get a release out. Apologies for the delay in getting to this. If you can fix the merge conflict, I'll run it across the regression suite and get it in the next release.

Comment on lines +272 to +287
var key = a.TypeString;
if (key.Length < 1)
{
return;
}
if (key[0] == 0xa9 || key[0] == 0x40)
{
//Tag ID's beginning with the copyright symbol (hex 0xa9) are multi-language text
//Alternate language tags are accessed by adding a dash followed by a 3-character ISO 639-2 language code to the tag name.

//some stupid Ricoh programmer used the '@' symbol instead of the copyright symbol in these tag ID's for the Ricoh Theta Z1 and maybe other models

//For now we don't support those, we will strip the copyright and locale info
key = key.Substring(1);
key = key.Split('-')[0];
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

TypeString is produced from a uint and can only be four characters max. I can't see how you'd get a 2-letter language code in this string.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I updated the comments , you are completely right that is no way more than 4 chars can be in a unit 👍

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

In that case, why is there any need to do the substring/split operation? I don't understand why this code is here.

Do you have a sample image the requires this code in order to work? None of the images in the regression suite hit this code path. I think it can be removed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The related pull-request drewnoakes/metadata-extractor-images#51 adds a test-case video called "comment_in_metadata_itemlist_inside_userdata.mp4" which includes the copyright named tag cmt holding the comment "Lat: 51.904443422...." when debugging you can find:
image

The alternative solution is not to have the subsring but than add the copyright version of cmt in the QuickTimeMetadataHeaderDirectory _nameTagMap on line 129. I thought this would be a cleaner solution.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks, I missed that PR.

However the value is still only four characters and doesn't contain a -. Why do we need the code here that I commented on?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed the not required split operation.

# Conflicts:
#	MetadataExtractor/Formats/QuickTime/QuickTimeMetadataReader.cs
@drewnoakes
Copy link
Copy Markdown
Owner

I fixed the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants