Skip to content

Commit

Permalink
Remove problematic XMP code.
Browse files Browse the repository at this point in the history 
It's not currently possible to reliably map XMP's namespace/path
structure to metadata-extractor's integer-based tag identifier system.

The current code attempts to map a few properties to tags, but users
who rely on these commonly miss other valuable tags. This partial
XMP support has been the cause of many issues over the years, and
I've been considering removing this support for a long time.

Long term, I want the library to allow each directory a means to identify
their own tags. The integer system stems from starting with TIFF/Exif.
Until that time, I think it's better to remove this code. Much of it is
commented out and incomplete.

Some users might be surprised to see these tags no longer exist,
but I believe that by using the XMPMeta object they'll be better
served in the long term.

I can imagine providing some constants and helper methods for
working with XMPCore, if needed.
  • Loading branch information
@drewnoakes
drewnoakes committed Jan 25, 2017
1 parent d031a25 commit 5b07a49
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 927 deletions.
116 changes: 0 additions & 116 deletions Source/com/drew/metadata/xmp/XmpDescriptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,9 @@
*/
package com.drew.metadata.xmp;

import com.drew.imaging.PhotographicConversions;
import com.drew.lang.Rational;
import com.drew.lang.annotations.NotNull;
import com.drew.lang.annotations.Nullable;
import com.drew.metadata.TagDescriptor;

import static com.drew.metadata.xmp.XmpDirectory.*;

/**
* Contains all logic for the presentation of xmp data, as stored in Xmp-Segment. Use
* this class to provide human-readable descriptions of tag values.
Expand All @@ -37,119 +32,8 @@
@SuppressWarnings("WeakerAccess")
public class XmpDescriptor extends TagDescriptor<XmpDirectory>
{
// TODO some of these methods look similar to those found in Exif*Descriptor... extract common functionality from both

public XmpDescriptor(@NotNull XmpDirectory directory)
{
super(directory);
}

/** Do some simple formatting, dependant upon tagType */
@Override
public String getDescription(int tagType)
{
switch (tagType) {
case TAG_MAKE:
case TAG_MODEL:
return _directory.getString(tagType);
case TAG_EXPOSURE_TIME:
return getExposureTimeDescription();
case TAG_EXPOSURE_PROGRAM:
return getExposureProgramDescription();
case TAG_SHUTTER_SPEED:
return getShutterSpeedDescription();
case TAG_F_NUMBER:
return getFNumberDescription();
case TAG_LENS:
case TAG_LENS_INFO:
case TAG_CAMERA_SERIAL_NUMBER:
case TAG_FIRMWARE:
return _directory.getString(tagType);
case TAG_FOCAL_LENGTH:
return getFocalLengthDescription();
case TAG_APERTURE_VALUE:
return getApertureValueDescription();
default:
return super.getDescription(tagType);
}
}

/** Do a simple formatting like ExifSubIFDDescriptor.java */
@Nullable
public String getExposureTimeDescription()
{
final String value = _directory.getString(TAG_EXPOSURE_TIME);
if (value==null)
return null;
return value + " sec";
}

/** This code is from ExifSubIFDDescriptor.java */
@Nullable
public String getExposureProgramDescription()
{
return getIndexedDescription(TAG_EXPOSURE_PROGRAM,
1,
"Manual control",
"Program normal",
"Aperture priority",
"Shutter priority",
"Program creative (slow program)",
"Program action (high-speed program)",
"Portrait mode",
"Landscape mode"
);
}


/** This code is from ExifSubIFDDescriptor.java */
@Nullable
public String getShutterSpeedDescription()
{
final Float value = _directory.getFloatObject(TAG_SHUTTER_SPEED);
if (value==null)
return null;

// thanks to Mark Edwards for spotting and patching a bug in the calculation of this
// description (spotted bug using a Canon EOS 300D)
// thanks also to Gli Blr for spotting this bug
if (value <= 1) {
float apexPower = (float) (1 / (Math.exp(value * Math.log(2))));
long apexPower10 = Math.round((double) apexPower * 10.0);
float fApexPower = (float) apexPower10 / 10.0f;
return fApexPower + " sec";
} else {
int apexPower = (int) ((Math.exp(value * Math.log(2))));
return "1/" + apexPower + " sec";
}
}

/** Do a simple formatting like ExifSubIFDDescriptor.java */
@Nullable
public String getFNumberDescription()
{
final Rational value = _directory.getRational(TAG_F_NUMBER);
if (value==null)
return null;
return getFStopDescription(value.doubleValue());
}

/** This code is from ExifSubIFDDescriptor.java */
@Nullable
public String getFocalLengthDescription()
{
final Rational value = _directory.getRational(TAG_FOCAL_LENGTH);
return value == null ? null : getFocalLengthDescription(value.doubleValue());
}

/** This code is from ExifSubIFDDescriptor.java */
@Nullable
public String getApertureValueDescription()
{
final Double value = _directory.getDoubleObject(TAG_APERTURE_VALUE);
if (value==null)
return null;
double fStop = PhotographicConversions.apertureToFStop(value);
return getFStopDescription(fStop);
}
}
Loading

16 comments on commit 5b07a49

@rcketscientist
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this includes some of my changes from back in the day. What's the new use-case? Are you referring to using the Adobe library directly?

@drewnoakes
Copy link
Owner Author

Choose a reason for hiding this comment

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

That's right, the XMPCore API does a better job of exposing the XMP data. There was regular confusion as to why certain XMP values were not present in the output, yet others were.

@rcketscientist
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'll have to see what that looks like when I update (prolly just revert the file atm for time constraints). At a glance this should be a major version, right (no backwards compatibility)?

@drewnoakes
Copy link
Owner Author

Choose a reason for hiding this comment

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

XMPMeta is easy to work with. I'd be surprised (and interested) if it's a big change for you to use the new API.

This project predates the current semver hotness. Here for version A.B.C, then changes to C means no API change, B means minor changes, and A means considerable changes.

@rcketscientist
Copy link
Contributor

@rcketscientist rcketscientist commented on 5b07a49 Feb 12, 2017 via email

Choose a reason for hiding this comment

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

@drewnoakes
Copy link
Owner Author

Choose a reason for hiding this comment

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

The XMP dependency is still useful as metadata-extractor essentially finds the XMP data within several file formats, and in some cases pulls together multiple fragments into a single parseable document.

As for a generic interface, I really like the idea but haven't yet found a good implementation. There's an existing issue [#10) that covers this in essence, and you're right -- it would likely reduce code duplication in the wild, or at the very least increase the quality of of the most common requirements users have.

Such an API could be layered on top of an Metadata object, to keep a clear separation. Something like new MetadataDecoder(metadata).getIso(). Any thoughts (at the issue) such as which properties you'd want, or even a PR once your release is out would be great.

@rcketscientist
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking BetaMeta ;-). I think it would be quite a while before people agreed on a set of tags as "official" so an API (name)change down the road to signal maturity would probably be fine. I probably already have a very rough framework to donate. #10 is pretty dead-on. I'll add my two cents there.

@xeruf
Copy link

@xeruf xeruf commented on 5b07a49 Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I am currently updating this library on old code, and since I am not entirely familiar with it, I can't seem to figure out the proper replacement for this:

XmpDirectory.getDate(XmpDirectory.TAG_CREATE_DATE)

Could you assist me?

@rcketscientist
Copy link
Contributor

@rcketscientist rcketscientist commented on 5b07a49 Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Here's how I grab a simple xmp field:

        private static String getLabel(Metadata meta)
	{
		for (Directory dir : meta.getDirectoriesOfType(XmpDirectory.class))
		{
			XMPMeta xmp = ((XmpDirectory)dir).getXMPMeta();
			try
			{
				return xmp.getPropertyString(LABEL.Schema, LABEL.Name);
			}
			catch (XMPException e)
			{
				// do nothing
			}
		}
		return null;
	}

Schema and name are stored in my case, but they're just the strings you woudl expect.

@drewnoakes
Copy link
Owner Author

Choose a reason for hiding this comment

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

For TAG_CREATE_DATE see line 185 that suggests the property name is "xmp:CreateDate". I believe the namespace is "http://ns.adobe.com/xap/1.0/".

@xeruf
Copy link

@xeruf xeruf commented on 5b07a49 Feb 28, 2018

Choose a reason for hiding this comment

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

yeah I figured it out it's getXMPMeta().getPropertyDate(Schema.XMP_PROPERTIES, "CreateDate")

@sghpjuikit
Copy link

Choose a reason for hiding this comment

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

I realize this is an old post, but allow me to chip in. I too noticed this change only now.

The current code attempts to map a few properties to tags, but users who rely on these commonly miss other valuable tags

Assuming the properties can not be enumerated, would it not make sense to attempt to provide constants for as many properties as possible? Isn't incomplete type safety better than none at all? Isn't an issue resulting from using the constants on the user/developer? What am I missing?

@drewnoakes
Copy link
Owner Author

Choose a reason for hiding this comment

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

Discussed elsewhere but probably easier to summarise here than find the info.

People kept asking for tags to be added for this or that.

The XMP data uses a non-integral key for each property value, which doesn't map to the current Directory object's structure well (I would like to change that in future).

Most 'honest' thing to do was to remove the few tags that were there and encourage people who want to use XMP to use it to the fullest via the XMPCore library.

Probably not satisfactory for everyone, but the clearest route on balance.

@rcketscientist
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be tooting my own horn (and probably wrongly given my memory at this point), but I think the Sigma work I did would allow us to eventually introduce more versatile keying if we ever get a chance to look at that in the scope of things.

@xeruf
Copy link

@xeruf xeruf commented on 5b07a49 Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I just wondered why you remapped all that stuff instead of simply defining an XMP enum containing all the common keys each referring to the respective String?

@rcketscientist
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the common keys? How exactly should they be implemented in relation to bit-wise keys? I'm guessing you never worked with maker notes or you're a millennial...or both. Truth is, if it's as simple as you say you can PR. If you want to see what's necessary to implement versatile keys and backwards compatibility take a look at https://github.com/drewnoakes/metadata-extractor/pull/261/files if you want to understand what you just flippantly asked.

Please sign in to comment.