Add support for EXIF 2.32

[JakeSmarter]

This PR also fixes some tag names and deprecates invalid tags.

[JakeSmarter]

This PR does not make ExifTagConstants fully standards compliant though (unfortunately). There are still tag constant values left which do not comply with the data type as outlined in the EXIF specification, like ExifTagConstants.EXPOSURE_PROGRAM_VALUE_MANUAL which is of type int while it should be short. Updating their data types to the correct data types is easy but this change may break backwards compatibility for some applications. Simply deprecating them is no option either because their replacements would need to be labeled with new identifiers which ultimately would create new inconsistencies and unnecessary mess. So, I did not touch these since breaking backwards compatibility would require developer consensus, even though commons-imaging—if I am not mistaken—has not reached version 1.0 status yet.

Should you have any idea how to fix these constants without breaking backwards compatibility, I would be happy to read about it.

[kinow]

Hi @gitne

Thanks a lot for your contribution. I am currently overseas, but planning to look into this PR as soon as I have some spare time.

On backward compatibility, thanks a lot for taking that into consideration. That's an important point.

We have added 1 backward incompatible change after the alpha1 release. Which means that the alpha2 release would not be backward compatible.

There was some discussion in the mailing list on whether it would be OK or not to add such changes in alpha releases. I believe for now we are going with a per-component policy, where alpha releases in Commons Imaging would not be behaviour/binary backward compatible (we still have time to discuss that before alpha2).

I'm writing to that thread about this PR, to confirm whether we are keeping BC or not. If not, it would be great if you would be willing to complete the work for fully compliance with the standard.

Thanks
Bruno

• https://markmail.org/thread/drivqucdadxzfb3i

[JakeSmarter]

On backward compatibility, thanks a lot for taking that into consideration. That's an important point.
[…]
I'm writing to that thread about this PR, to confirm whether we are keeping BC or not. If not, it would be great if you would be willing to complete the work for fully compliance with the standard.

After thinking about it a bit longer, I have realized that this PR already does break backwards compatibility in a specific situation. Namely, if an application relies on a deprecated TagInfo object to be in the ExifTagConstants.ALL_EXIF_TAGS list. For example, ExifTagConstants.ALL_EXIF_TAGS.contains(ExifTagConstants.EXIF_TAG_EXIF_IMAGE_WIDTH) will return false instead of true for them. So, if you would simply drop in a commons-imaging.jar with this patch applied on an existing application which expects the ExifTagConstants.EXIF_TAG_EXIF_IMAGE_WIDTH to be in ExifTagConstants.ALL_EXIF_TAGS then things will break. However, I believe this to be an exceptionally rare case, if ever implemented in this way. If anything ExifTagConstants.ALL_EXIF_TAGS has been probably used in iterators by applications, so these would still transparently iterate over the new and fixed TagInfo objects anyway. But they will misbehave if they check on a deprecated TagInfo object while iterating, yet they will not crash.

We can work around ExifTagConstants.ALL_EXIF_TAGS.contains() to return true on deprecated TagInfo objects, however we cannot fix the iterators because we do not know which TagInfo objects the caller expects. Hence, there is practically no way around breaking backwards compatibility. Nevertheless, it would be a good thing anyway since IMHO it is better for dependencies/libraries to be correct than backward compatible. Consequently, in that situation fixing the data types of those primitive static members can be done too.

Furthermore, ExifTagConstants.ALL_EXIF_TAGS should have probably been a Set in the first place because there is really no order among EXIF tags. If anything then the current List should be reordered by tag ids because there is no apparent order in the source code. So, either the lack of order should be reflected in the type or the ordered type should have elements in proper order. But this might be rather something for another PR or commit because both approaches will break backwards compatibility.

[kinow]

@gitne I think we are good with breaking changes before the final 1.0 release (see mailing archive: https://markmail.org/thread/dszscnnjpy5shu5f).

Feel free to complete the PR with any other changes you may find useful. Just let us know once it's ready for review 👍

And thanks for your contribution!
Bruno

[kinow]

One test failing on Travis:

[ERROR] Failures: 
[ERROR]   TiffTagIntegrityTest.testTagIntegrity:62->verifyFields:115 Missing tag 34867
[INFO] 
[ERROR] Tests run: 615, Failures: 1, Errors: 0, Skipped: 16
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

[coveralls]

Coverage increased (+0.05%) to 76.517% when pulling 38a6d9a on GITNE:exif_2.32 into 80e3079 on apache:master.

[JakeSmarter]

[ERROR] TiffTagIntegrityTest.testTagIntegrity:62->verifyFields:115 Missing tag 34867

Thank you for catching this. 👍 I have rebased the faulty commit.

I have one commit yet to come. It is just basically a reorder of ALL_*_TAGS lists by tag id.

Additionally, IMHO all non-standard tags should either be put into separate *TagConstants classes or an Extension nested class per IFD. So, for example GpsTagConstants should/could actually be a nested class of ExifTagConstants since it is an extension of Exif but in a separate IFD. Primitve non-standard tags which live in the Exif IFD should then go into the generic Extension nested class of ExifTagConstants. ExifTagConstants would then be a nested class of TiffTagConstants. Alternatively, instead of using nested classes inheritance could be employed to model the relationships between tag sets.
So, why might this be useful? The current code model produces ambiguous tag names, like EXIF_TAG_WHITE_BALANCE_1, EXIF_TAG_WHITE_BALANCE_2, … etc. Currently, EXIF_TAG_WHITE_BALANCE_1 is the name of the Exif standard tag and EXIF_TAG_WHITE_BALANCE_2 is a vendor specific non-standard extension to white balance metadata. Library consumers have basically no way of figuring out which one is which, unless they look into the source code or reverse engineer the behavior by reading the tag id. So, I would like to change this too. However, this might be something for a separate PR, right?

[kinow]

Yeah, that's a not very nice issue finding the tags... but I think that's probably better done in another PR later.

[JakeSmarter]

The Travis CI job fails for some PR unrelated reason. Can you fix it?

Basically, this PR is done, unless you want me to change something.

[kinow]

@gitne it's a checkstyle issue. If you click on "Details" next to the Travis CI check (👇 ) there should be a few jobs within this PR's build.

Looking at a random job's log, I see:

[ERROR] src/main/java/org/apache/commons/imaging/formats/tiff/constants/ExifTagConstants.java:[0] (misc) NewlineAtEndOfFile: File does not end with a newline.

Looking at the changed files in this PR, it's possible to spot that your editor probably removed the empty line at the end of the file.

If you add that back, squash your commit, and push, Travis should kick again and hopefully pass this time 🤞

BTW, I've been holding back reviewing this one until you confirmed you got everything ready. I remember you mentioning you wanted to add something more... so just let us know once it's ready for review :)

Cheers
Bruno

[JakeSmarter]

I have added terminal newlines where applicable but Travis CI still fails for some unrelated reason.

Btw, personally I consider tools which require you to put a terminal newline in your files to be broken or to have a badly implemented parser. That pseudo argument for terminal newlines where stream processing concatenated files my lead to undesired behavior is dumb. It is naive to assume that processing concatenated files and processing files separately will or should always produce identical results. It is the file format that governs parsing not the mode of processing.

[kinow]

Hi @gitne

I have added terminal newlines where applicable but Travis CI still fails for some unrelated reason.

You have to rebase your branch on master. I've fixed the Javadoc error on JVM 13. So once rebased, this PR should pass Travis with no issues 🤞 . Do you intend to modify anything else in the PR? Add any feature/code?

Btw, personally I consider tools which require you to put a terminal newline in your files to be broken or to have a badly implemented parser. That pseudo argument for terminal newlines where stream processing concatenated files my lead to undesired behavior is dumb. It is naive to assume that processing concatenated files and processing files separately will or should always produce identical results. It is the file format that governs parsing not the mode of processing.

Just to clarify, the newline here is for git. I believe it came from unix where it was good practice to have the newline. Not sure if there's a practical reason for that.

[kinow]

Rebased, and tried to fix conflicts. @gwlucastrig some constants changed here are related to your previous change in TIFF constants class, maybe you can confirm they look good too, from what you remember?

Will re-read the PR later, with more calm, and see if it's ready to be merged or if it needs any further work. Thanks @gitne