Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complete easy access coverage for Exif & TIFF/EP #1385

Closed
kmilos opened this issue Nov 2, 2020 · 20 comments · Fixed by #1386
Closed

Complete easy access coverage for Exif & TIFF/EP #1385

kmilos opened this issue Nov 2, 2020 · 20 comments · Fixed by #1386
Assignees
Labels
request feature request or any other kind of wish
Milestone

Comments

@kmilos
Copy link
Collaborator

kmilos commented Nov 2, 2020

Describe the solution you'd like
There are a few more tags that overlap between Exif and TIFF/EP that are not covered in src/easyaccess.cpp (e.g. ShutterSpeedValue, ApertureValue, etc.)

It would be great to complete the coverage to avoid users having to manually check both Exif.Photo and Exif.Image IFDs.

This would help improve support of DNGs for example.

@kmilos kmilos added the request feature request or any other kind of wish label Nov 2, 2020
@kmilos
Copy link
Collaborator Author

kmilos commented Nov 2, 2020

There are still a few remaining, but maybe very niche?

OECF
SpectralSensitivity
SpatialFrequencyResponse
FocalPlaneXResolution
FocalPlaneYResolution
FocalPlaneResolutionUnit

The only interesting left is maybe DateTimeOriginal? (added)

@clanmills
Copy link
Collaborator

I've never understood the purpose of the "easyaccess", so I don't have an opinion about what should or shouldn't be included.

The exiv2 --grep option is useful to specify tags when you're not sure if they are in the Tiff IFD (which exiv2 calls Exif.Image.Tag) or the Exif IFD (which exiv2s calls Exif.Photo.Tag) or the Makernote (which exiv2 calls something like Exif.Canon.Tag). If you have a collection of tags that you frequently inspect, you could:

$ export MY_EXIV2_TAGS='--grep this --grep that --grep other'
$ exiv2 $MY_EXIV2_TAGS foo.dng

If you want to prepare a PR with the necessary code, updates to the test suite, and user documentation (man page exiv2.1 and/or the web-site), I'll be happy to review and merge your code into the 0.27-maintenance branch.

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 2, 2020

This mostly helps developers using the API, not in the CLI. See e.g. darktable-org/darktable#6686

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 2, 2020

Is the web API documentation automatically generated? If so, than it should already be covered by the header file updates and comments.

There is no impact on the man page AFAICT.

That leaves the test suite - is it just a matter of updating samples/easyaccess-test.cpp?

@clanmills
Copy link
Collaborator

@kmilos As you know, Miloš, there's aways more that one way to do everything. You could add to the C++ unittests, update samples/easyaccess-test.cpp, or add to the python_tests. Extending samples/easyaccess-test.cpp appears to be the quickest way to do this. It's invoked from the bash script test/iso65k-test.sh.

We have a project underway (and mostly finished) to replace the bash scripts with python code in tests/bash_tests. The rational for the "pythonic bash tests" is in the project proposal #1215. Conceptually, the tests work in the same way as the "real" bash scripts in test/*.sh. @LeoHsiao1 owns that task and has done a really good job.

We'll have to port iso65k-test.sh to the new python environment. The obvious division of effort here is for you to update test/iso65k-test.sh and its reference file data/iso65k-test.out. @LeoHsiao1 can you port test/iso65k-test.sh to tests/bashtests, please?

I'll be honest and say that this morning I forget what easyaccess does. I was thinking of the default output of exiv2 which prints a subset of metadata. That's not what easyaccess is about. It's those APIs in easyaccess.hpp. So no changes are necessary in the man page exiv2.1. However, I think we should add a document to the Wiki to explain the easyaccess APIs.

The API pages on exiv2.org are generated by Doxygen which reads code comments and generates HTML (and UML diagrams and other magic).

When the email arrived about #1386 arrived, it listed changes to 300 files. Presumably that dropped to 2 when you changed the base branch to 0.27-maintenance. Two files. That feels better!

These changes will get into 'master' in time. The release engineer for v0.28 (probably me) will have to port lots of changes from 0.27-maintenance into 'master'. I hope this will be done in Spring 2021.

@clanmills
Copy link
Collaborator

I'm rather surprised by this. In 12+ years of working on Exiv2, today might be the first time anybody's ever mentioned the easyaccess API. I found a site with a draft of the DNG standard. Is the purpose EasyAccess to give a simple way to access the keys blessed in the DNG spec? http://www.barrypearson.co.uk/top2009/downloads.htm

In my book (which is work in progress) I did raise the subject of Tags in DNG that are not discussed in Tiff6.0. https://clanmills.com/exiv2/book/#TIFF

I was correct in believing the easyaccess API is used to report some of the information in the "default" exiv2 report. However the default report isn't documented in the man page exiv2.1.

The only references to this on the web-site are doxygen generated:

https://exiv2.org/doc/files.html
https://exiv2.org/doc/easyaccess_8hpp.html

549 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ exiv2  http://clanmills.com/Stonehenge.jpg
File name       : http://clanmills.com/Stonehenge.jpg
MIME type       : image/jpeg
Image size      : 6000 x 4000
Camera make     : NIKON CORPORATION
Camera model    : NIKON D5300
Image timestamp : 2015:07:16 15:38:54
Image number    : 
Exposure time   : 1/400 s
Aperture        : F10
Exposure bias   : 0 EV
Flash           : No, compulsory
Flash bias      : 
Focal length    : 44.0 mm (35 mm equivalent: 66.0 mm)
Subject distance: 
ISO speed       : 200
Exposure mode   : Not defined
Metering mode   : Multi-segment
Macro mode      : 
Image quality   : NORMAL 
Exif Resolution : 6000 x 4000
White balance   : AUTO        
Thumbnail       : image/jpeg, 10837 Bytes
Copyright       : 
Exif comment    : charset=Ascii                                     

550 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ 

I think I see a new topic for the book appearing from the swamp.

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 3, 2020

When the email arrived about #1386 arrived, it listed changes to 300 files. Presumably that dropped to 2 when you changed the base branch to 0.27-maintenance. Two files. That feels better!

Yeah, sorry about that! You'd think github would figure out which base to pick automatically according to where I branched off. I feel for you @clanmills when the time comes to review 300+ files for the merge...

Don't know what the original intention was, but easy accessors make sense to me, given that different vendors and standard bodies put the same metadata into (potentially differently named) tags located over several IFDs. Leaving it to the users of the exiv2 library through the API to check every possibility would be cruel indeed - look at e.g. at isoSpeed() and how much more complicated this would be.

I've included the additions into samples/easyaccess-test.cpp and believe the PR should be complete. I presume easyaccess-test will be run as a binary independently, whether from shell or Python, but will let @LeoHsiao1 correct me if anything else is needed. There is however a failure on easyaccess-test now, so might need help to figure out what needs to change for the reference output.

@clanmills
Copy link
Collaborator

@kmilos Miloš, thanks for doing this and for your words of explanation. As the name "easyaccess" implies, its intention is to "best guess" about which key (Exif.Family.Tag) to use for a common camera feature such as Aperture (fNumber) and FocalLength (focalLength).

1185 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ nm -g --demangle build/lib/libexiv2.dylib | grep fNumber
000000000003fb00 T Exiv2::fNumber(Exiv2::ExifData const&)
1186 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ nm -g --demangle build/lib/libexiv2.dylib | grep focalLength
000000000003fbc0 T Exiv2::focalLength(Exiv2::ExifData const&)
1187 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ 

The code in src/actions.cpp printSummary() is a good example of the "jumbled confusing mess" that you've described. I wonder if we shouldn't rectify all of this now and use the easyaccess API in actions.cpp. As the man page doesn't document the output of the "printSummary" (-ps, which is the printing default), I don't think we need to touch exiv2.1

If you'd are willing to work on actions.cpp, I'll document easyaccess in the Wiki (and in my book). I'm sure @LeoHsiao1 will step up to dealing with porting iso65k-test.sh to tests/bashtests. Life's more fun when we share and cooperate.

I will try to contact the author of easyaccess. Carsten Pfeiffer pfeiffer@kde.org. This is both a courtesy and to invite his input and thoughts.

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 3, 2020

Thanks, I think I finally ironed out my easyacces_test related issues and PR #1386 is ready.

If you'd are willing to work on actions.cpp, I'll document easyaccess in the Wiki (and in my book). I'm sure @LeoHsiao1 will step up to dealing with porting iso65k-test.sh to tests/bashtests. Life's more fun when we share and cooperate.

Can't promise, but I'll take a look at actions.cpp in the coming weeks to see if I can handle it.

@clanmills clanmills self-assigned this Nov 3, 2020
@clanmills clanmills added this to the v0.27.4 milestone Nov 3, 2020
@clanmills
Copy link
Collaborator

@kmilos Don't bother working on actions.cpp. I've done that and added two new "easyaccess" selectors Exiv2::imageWidth() and Exiv2::imageHeight().

Having dealt with you several times, I would be happy to make you a member of Team Exiv2 and that will enable you to submit PRs to exiv2/exiv2. Other team members can edit them. This is our usual way of working.

Choice time:

  1. I send you a git diff for you to merge into your PR and you update Add more easy accessors for Exif & TIFF/EP (0.27->master) #1386.
  2. I grant you write privileges to exiv2/exiv2 and you can submit a new PR, which I (and @LeoHsiao1) can edit.
  3. I make you a member of Team Exiv2 and you will have write permissions.

I still have the following tasks to finish:

  1. output tweaks concerning focalLength() and subjectDistance() to pass test suite (without changing any reference output in test/data/*.out).
  2. review your code.
  3. read the DMG spec and see if there is anything else we should be concerned about here.
  4. write an Wiki Article about easyaccess. That text will also go in the book.
  5. have a look at the strangely named test/iso65k-test.sh
  6. email Carsen for feedback

I've spoken with @LeoHsiao1 about porting test/iso65k-test.sh to tests/bash_tests. He usually works on open source on Sundays, so it's probably going to take a couple of weeks to work on this.

My priority is to work on the book, so I hope to wrap up everything (except tests/bash_tests) this week.

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 4, 2020

Hi Robin, let's just add your diff to my existing PR. Unfortunately I don't think I can commit regular chunks of time to Team Exiv2, so I'll just be "lurking" occasionally if you don't mind...

Re DNG, I've referenced Barry's page as well, but maybe it's a bit dated now, better stick to the latest official published spec (1.5 ATM). I've also relied on https://www.loc.gov/preservation/digital/formats/fdd/fdd000073.shtml and https://www.loc.gov/preservation/digital/formats/content/tiff_tags.shtml

@clanmills
Copy link
Collaborator

That's fine. I'm not asking you to work on Exiv2. I'm wondering about the work-flow. So, you've made your choice: 1. I send you a diff for you to merge into your PR. Happy to have you lurking.

My changes to actions.cpp are ready. I decided to modify the output order of $ exiv2 -ps foo because it made the code in actions.cpp simpler. I removed the "Resolution: " output in $ exiv2 -ps foo because it was wrong and reported the image size (which it already reported as Image size:). So I haven't changed anything in easyaccess or src/actions.hpp.

The following comments in test/iso65k-test.sh explain the test:

# test for ISOs which follow Annex G of EXIF 2.3 spec, i.e. ISOs,
# which cannot be represented by Exif.Photo.ISOSpeedRatings due to
# being larger than 65k

# All tests use the summary view as a result-check, because it uses
# the isoSpeed() "easyaccess" function, which handles the higher ISO
# readout.

As almost all the output of $ exiv2 -ps foo is generated using the easyaccess API, our existing test harness is sufficient for your changes. The only outstanding code matter relating to easyaccess is for @LeoHsiao1 to port test/iso65k-test.sh to tests/bash_tests. That will be another PR and not involved with #1386.

I have started to work on the documentation and will contact Carsen. Again, that doesn't require you to wait.

So if you're happy with my changes to your PR, please update and we're done. A pleasure to work with you.

1386.patch.zip

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 4, 2020

So if you're happy with my changes to your PR, please update and we're done. A pleasure to work with you.

Same here! The patch looks fine, and I've made a commit to my local branch. Before I push, I also noticed a query for Exif.Canon.FileNumber - do you think this is the same as Exif.Image.ImageNumber (0x9211 in TIFF/EP)? If so, should we maybe also add it to the easyaccess API?

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 4, 2020

do you think this is the same as Exif.Image.ImageNumber (0x9211 in TIFF/EP)?

Actually, now it doesn't look it for me, Exif.Image.ImageNumber seems to be for a images in a same burst/sequence.

But there is also Exif.CanonFi.FileNumber and Exif.NikonFi.FileNumber... all with slightly different implementations it seems, so maybe I skip this one.

I have just tweaked your patch to print "File number" instead of "Image number" to avoid this and future confusion, I hope this is ok?

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 4, 2020

Sorry, maybe I was a bit too quick on the patch review: I think there is still a case for the 2 fallbacks even with the easyaccess API: exposureTime/shutterSpeedValue and fNumber/apertureValue, they won't get substituted automagically...

@clanmills
Copy link
Collaborator

A tag such as Exif.Image.ImageNumber was found in the Tiff IFD and will almost certainly be in an Adobe Spec. A tag such as Exif.Photo.Tag was found in the Exif IFD and should be in an Exif Spec.

Tags such as Exiv2.CanonFi.FileNumber and Exiv2.NikonFi.FileNumber are derived from the MakerNote (which is usually an IFD), so they are unlikely to be in the DNG Spec because the Manufacturers all do what suits them and overlap is a coincidence. I wish Andreas hadn't create all those different Groups relating to a manufacturer and had just called them Exiv2.Canon.FileNumber or Exiv2.Nikon.FileNumber. A good example in src/easyaccess.cpp is:

    ExifData::const_iterator flashBias(const ExifData& ed)
    {
        static const char* keys[] = {
            "Exif.CanonSi.FlashBias",
            "Exif.Panasonic.FlashBias",
            "Exif.Olympus.FlashBias",
            "Exif.OlympusCs.FlashExposureComp",
            "Exif.Minolta.FlashExposureComp",
            "Exif.SonyMinolta.FlashExposureComp",
            "Exif.Sony1.FlashExposureComp",
            "Exif.Sony2.FlashExposureComp"
        };
        return findMetadatum(ed, keys, EXV_COUNTOF(keys));
    }

I don't know what 'flashBias' is, however it only shows up in MakerNotes. It's not in Tiff/DNG/Adobe IFD and not in the Exif IFD.

Q. What's the difference between Exif.Sony1.FlashExposureComp and Exif.Sony2.FlashExposureComp?
A. Very little. Different versions of the MakeNote in different Sony cameras.

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 4, 2020

I understand the tag location/group differences (main IFD for TIFF/EP or DNG and others, vs Exif IFD, vs vendor MakerNote IFD), The point is that easyaccess API was trying to unify ones that are supposed to store corresponding values in (hopefully) identical implementations, such as e.g.

    ExifData::const_iterator subjectDistance(const ExifData& ed)
    {
        static const char* keys[] = {
            "Exif.Photo.SubjectDistance",
            "Exif.Image.SubjectDistance",
            "Exif.CanonSi.SubjectDistance",
            "Exif.CanonFi.FocusDistanceUpper",
            "Exif.CanonFi.FocusDistanceLower",
            "Exif.MinoltaCsNew.FocusDistance",
            "Exif.Nikon1.FocusDistance",
            "Exif.Nikon3.FocusDistance",
            "Exif.NikonLd2.FocusDistance",
            "Exif.NikonLd3.FocusDistance",
            "Exif.Olympus.FocusDistance",
            "Exif.OlympusFi.FocusDistance",
            "Exif.Casio.ObjectDistance",
            "Exif.Casio2.ObjectDistance"
        };
        return findMetadatum(ed, keys, EXV_COUNTOF(keys));
    }

which is present in any of the three possible IFDs, or otherwise abstract to a common implementation like isoSpeed() does.

It doesn't seem Canon and Nikon implement FileNumber the same way, or that it matches ImageNumber in TIFF/EP spec (which is ISO spec btw, not only Adobe's like DNG), so let's drop it for now.

And I don't think there's anything really wrong with all those separate vendor groups. They really do seem to be independent and different SubIFDs of the MakerNote IFD (see e.g. exiftool's list for Canon).

@clanmills
Copy link
Collaborator

You're quite right. TIFF/EP, ISO 12234-2:2001 is an ISO spec (with strong input from Adobe). I'm a retired Adobe Engineer and was speaking with my old team hat!

The only point I'm making about the Vendor sub-groups is that everything would appear simpler as "Exif.Sony.Tag" as there really isn't a need for Exif.Sony1.Tag and Exif.Sony2.Tag. Anyway, that's how it is.

So, I'm not too sure what point you're making here. It seems to me that if you use easyaccess for "subjectDistance", it will try each of these keys in turn (or return exifData.end()) and that seems quite smart as it's searching TIFF/EP IFD, then Exif IFD, then various MakerNotes.

Reporting "File number" instead of "Image number" sounds good. Thanks for dealing with that.

I feel good about everything here. If you're uneasy, let's talk more. I was mistaken in thinking that @LeoHsiao1 had work to complete concerning iso65k-test in tests/bash_tests. He dealt with that in September. If you're happy, please update #1386 and we'll get this closed today.

@clanmills
Copy link
Collaborator

@kmilos
Copy link
Collaborator Author

kmilos commented Nov 5, 2020

I think we've beaten this to death. Time to move on!

Agreed, and the wiki looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request feature request or any other kind of wish
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants