-
Notifications
You must be signed in to change notification settings - Fork 281
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
fix_929_exif2.32 (0.27->master) #1389
Conversation
Ah, I just realized the easyacces API in #1385 should've also mentioned the time zone offsets, sorry I missed that. Exif's
I have no idea ATM on how to handle this case in the easyaccess API, as an iterator to the entire |
Relax. I have two immediate thoughts about this:
|
It could be even easier. According to the Wiki, the following the easyaccess function are: https://github.com/Exiv2/exiv2/wiki/EasyAccess-API
If you want add timeZoneOffset (or something) it can look look for OffsetTime, then TimeZoneOffset. The caller will have to inspect the type/count of the returned pointer, as it could be ascii or shorts. |
I've had another thought. EasyAccess is intended to make it easy to access some metadata. It's not easy to access the TZ information! So, the easyaccess API in #1385 works as documented in the Wiki. With this PR, Exiv2 provides support for Exif 2.31 tags OffsetTime +Original +Digitized. Exiv2 has supported the TIFF/EF tag TimeZoneOffset for sometime. |
Indeed, so I think it's best left out for now. |
I had a totally new thought about this while running in the forest this morning (good place to think). We could have another 'layer' of code in Exiv2 between the library and applications in the file src/exiv2glue.cpp. We could have define APIs such as In this way we could add/modify APIs without changing the library API. This is C++ code. If users want to use Exiv2Glue functions, they have to compile/link src/exiv2glue.cpp with their application. |
Nice idea! Though since it's a departure from the "established" 0.27 features, I'd write it down on the 0.28 wishlist? In any case, nobody seems to be missing this ATM... |
I'd like to merge and close this. Please accept the invitation from GitHub to join Team Exiv2 and I can assign you as the reviewer. Last week, I asked Luis to review this, however nothing has happened. I can disable the setting to "require approvals", however it's better that changes are approved. |
src/tags_int.cpp
Outdated
@@ -834,6 +834,12 @@ namespace Exiv2 { | |||
"version of a TIFF/EP file, eg '1', '0', '0', '0'"), | |||
ifd0Id, tiffEp, unsignedByte, 4, printValue), // TIFF/EP Tag | |||
TagInfo(0x9217, "SensingMethod", N_("Sensing Method"), N_("Type of image sensor."), ifd0Id, tiffEp, unsignedShort, 1, printValue), // TIFF/EP tag | |||
TagInfo(0x9400, "Temperature", N_("Temperature"), N_("Shooting situation."), ifd0Id, otherTags, signedRational, 1, printValue), // Exif 2.31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tags 0x9400 through to 0x9405 should be moved to Exif only section, and be part of exifId (not ifd0Id), like the others.
Also some comments should be fixed: 2 "TExif" typos, and 0x9405 is also "Exif 2.31" and not "TIFF/EP"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the 0x9217 tag is part of this PR. I agree that the comment is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The Exif2.3 tags should be in exifId.
I've been distracted by the tiffEp,otherTags and other values. I can't remember the purpose of that field. The comment in tags_int.hpp says:
/*!
@brief Section identifiers to logically group tags. A section consists
of nothing more than a name, based on the Exif standard.
*/
enum SectionId { sectionIdNotSet,
imgStruct, recOffset, imgCharacter, otherTags, exifFormat,
exifVersion, imgConfig, userInfo, relatedFile, dateTime,
captureCond, gpsTags, iopTags, mpfTags, makerTags, dngTags, panaRaw,
tiffEp, tiffPm6, adobeOpi,
lastSectionId };
src/tags_int.cpp
Outdated
@@ -1925,6 +1949,9 @@ namespace Exiv2 { | |||
TagInfo(0x001e, "GPSDifferential", N_("GPS Differential"), | |||
N_("Indicates whether differential correction is applied to the GPS receiver."), | |||
gpsId, gpsTags, unsignedShort, 1, print0x001e), | |||
TagInfo(0x001f, "GPSHPositioningError", N_("GPS Horizontal positioning error"), | |||
N_("This tag indicates horizontal positioning errors in meters."), | |||
gpsId, gpsTags, unsignedRational, 1, print0x001e), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want printValue here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
src/tags_int.cpp
Outdated
N_("This tag indicates whether the recorded image is a composite image* or not."), | ||
exifId, captureCond, unsignedShort, 1, printCompositeImage), | ||
TagInfo(0xa461, "SourceImageNumberOfCompositeImage", N_("Source Image Number Of Composite Image"), // Exif 2.32 | ||
N_("This tag indicates the distance to the subject."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description not updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
src/tags_int.cpp
Outdated
@@ -1752,6 +1767,15 @@ namespace Exiv2 { | |||
N_("This tag records the serial number of the interchangeable lens " | |||
"that was used in photography as an ASCII string."), | |||
exifId, otherTags, asciiString, 0, printValue), | |||
TagInfo(0xa460, "CompositeImage", N_("Composite Image"), // Exif 2.32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer comments after TagInfo() closing parenthesis for the next 3 tags, like in the rest of the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I've understood your comment.
src/tags_int.cpp
Outdated
N_("This tag indicates the distance to the subject."), | ||
exifId, captureCond, unsignedShort, 2, printValue), | ||
TagInfo(0xa462, "SourceExposureTimesOfCompositeImage", N_("Source Exposure Times Of Composite Image"), // Exif 2.32 | ||
N_("This tag indicates the distance to the subject."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description not updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
src/tags_int.cpp
Outdated
exifId, captureCond, unsignedShort, 2, printValue), | ||
TagInfo(0xa462, "SourceExposureTimesOfCompositeImage", N_("Source Exposure Times Of Composite Image"), // Exif 2.32 | ||
N_("This tag indicates the distance to the subject."), | ||
exifId, captureCond, undefined, 1, print0xa40c), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Count should be -1?
Also, the pretty-print function is wrong (copy/paste error?), probably want printValue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should be -1?
@kmilos Thank you for looking at this and your insightful comments. Winding my way through your comments one at a time is difficult. Can you edit the PR, please? Or submit a new PR. |
I see what SectionId is about. In the Exif2.32 spec there's a table in 4.6.4. These are tags in IFD0. A. Tags relating to image data structure 4.6.5 has tags in the Exif IFD. A. Tags Relating to Version Exif version 4.6.6 has tags in the GPS IFD. 4.6.7 has tags in the Interoperability IFD I'll need to study this and resubmit the PR. For sure, SectionId is used sparingly in Exiv2. Perhaps it was added as "might be useful one day" and is now confusing. 592 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ find . -name "*pp" | xargs grep SectionId
./src/tags.cpp: { lastSectionId, "(LastSection)", N_("Last section") }
./src/tags_int.hpp: enum SectionId { sectionIdNotSet,
./src/tags_int.hpp: lastSectionId };
./src/tags_int.hpp: SectionId sectionId_; //!< Section id
593 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ It must remain in the code because it's exposed in the API: 595 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ find . -name "*pp" | xargs grep sectionName
./include/exiv2/tags.hpp: static const char* sectionName(const ExifKey& key);
./samples/key-test.cpp: // sectionName
./samples/key-test.cpp: if (strcmp(ExifTags::sectionName(ek), "Interoperability") != 0) {
./samples/key-test.cpp: std::cout << "Testcase failed (sectionName)" << std::endl;
./samples/key-test.cpp: // sectionName
./samples/key-test.cpp: if (strcmp(ExifTags::sectionName(ek2), "Interoperability") != 0) {
./samples/key-test.cpp: std::cout << "Testcase failed (sectionName)" << std::endl;
./src/tags.cpp: const char* ExifTags::sectionName(const ExifKey& key) |
That's right, but not only in IFD0, the Exif IFD is also divided in sections (Table 4.6.5). I think most of them are already correct, apart from the 5 new 2.31 tags relating to "G2 shooting situation". I'm ok with those remaining in the "otherTags" section, otherwise I think this is the only new section needed... I assumed it was used in the code to further group tags into said sections when printing. I'll let you have another go at the PR changes if that was your plan, and then I'll review again and make further changes directly if necessary... |
I'll fix the PR on Monday and you can look at it. I'll also make changes to the book concerning this. At the moment, the book says the subject of the Tiff tags (in the Tiff 6.0 Spec) and DNG and Exif tags requires more investigation. I'll get the PR fixed and you can review. I've put everything I know into the book and yet new something new appears every week. For sure, the book is needed for future maintenance or to write a new metadata engine. |
…the "section" enum is of much importance. I don't believe anything in particular is done with with it. ```cpp /*! @brief Section identifiers to logically group tags. A section consists of nothing more than a name, based on the Exif standard. */ enum SectionId { sectionIdNotSet, imgStruct, recOffset, imgCharacter, otherTags, exifFormat, exifVersion, imgConfig, userInfo, relatedFile, dateTime, captureCond, gpsTags, iopTags, mpfTags, makerTags, dngTags, panaRaw, tiffEp, tiffPm6, adobeOpi, lastSectionId }; ``` I pleased with the documentation and drawing I have added to the book concerning the Exif Specification. https://clanmills.com/exiv2/book/#Exif
src/tags_int.cpp
Outdated
@@ -833,7 +833,13 @@ namespace Exiv2 { | |||
N_("Contains four ASCII characters representing the TIFF/EP standard " | |||
"version of a TIFF/EP file, eg '1', '0', '0', '0'"), | |||
ifd0Id, tiffEp, unsignedByte, 4, printValue), // TIFF/EP Tag | |||
TagInfo(0x9217, "SensingMethod", N_("Sensing Method"), N_("Type of image sensor."), ifd0Id, tiffEp, unsignedShort, 1, printValue), // TIFF/EP tag | |||
TagInfo(0x9217, "SensingMethod", N_("Sensing Method"), N_("Type of image sensor."), exifId, tiffEp, unsignedShort, 1, printValue), // TIFF/EP tag | |||
TagInfo(0x9400, "Temperature", N_("Temperature"), N_("Temperature as the ambient situation at the shot, for example the room temperature where the photographer was holding the camera. The unit is degrees C"), exifId, captureCond, signedRational, 1, printValue), // Exif 2.31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still think tags 0x9400 to 0x9405 should be moved to exifTagInfo[] (just like OffsetTime etc.), they really are Exif specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. You are totally correct. Yes. Let's do that.
src/tags_int.cpp
Outdated
@@ -833,7 +833,13 @@ namespace Exiv2 { | |||
N_("Contains four ASCII characters representing the TIFF/EP standard " | |||
"version of a TIFF/EP file, eg '1', '0', '0', '0'"), | |||
ifd0Id, tiffEp, unsignedByte, 4, printValue), // TIFF/EP Tag | |||
TagInfo(0x9217, "SensingMethod", N_("Sensing Method"), N_("Type of image sensor."), ifd0Id, tiffEp, unsignedShort, 1, printValue), // TIFF/EP tag | |||
TagInfo(0x9217, "SensingMethod", N_("Sensing Method"), N_("Type of image sensor."), exifId, tiffEp, unsignedShort, 1, printValue), // TIFF/EP tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x9217 should stay associated w/ ifd0Id, this one is TIFF/EP specific. There is a separate (and equivalent) 0xa217 for the Exif IFD.
But this one should probably also use print0xa217. Slightly different unfortunately, we need new translation values :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's use print0xa217().
src/tags_int.cpp
Outdated
{ 3, N_("CompositeCapturedWhenShooting") } | ||
}; | ||
|
||
std::ostream& printCompositeImage(std::ostream& os, const Value& value, const ExifData* metadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the macro EXV_PRINT_TAG() instead, no need to clutter the API with functions that are almost never used...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The code was cut'n'pasted without really thinking/understanding how everything fits together. We shouldn't add function to the API without good reason.
@kmilos Thanks very much for the thoughtful review. 2 heads are much better than one. In software (as in life) 1+1 == 3. Can you edit the PR and let's see the back of Exif 2.32 support. It has been a very successful and productive week. Best week I've had (both on the book and the project) since mid-June when I got tvisitor.cpp working. Very optimistic that the book will finished by the end of the year. |
Ok, I'll push my proposal in the coming days when I find some time. Btw, do you maybe have access to the official, published TIFF/EP spec to confirm the difference between TIFF/EP 0x9217 and Exif 0xa217 values? I only have the draft... ExifTool also shows a difference btw, so I'll assume it's real... |
TIFF-EP I have ISO/DIS 12234-2 which doesn't look like a draft. I got it from here: http://www.barrypearson.co.uk/top2009/downloads.htm 5.2.16 SensingMethod 0 Undefined Exif 2.32 I have CIPA DC-008-2019 which also doesn't appear to be a draft. I got it here. https://fotomagazin.hu/wp-content/uploads/2020/05/CIPA_DC_008_EXIF_2019.pdf SensingMethod Difference Caution I am dyslexic. TIFF-EF defines value 6. print0x217() is in the API.
|
Thanks for checking. That is the same draft I have (don't think Mr Pearson would be allowed to distribute a published ISO standard), so we'll go with that. It's not only addition of 6 in TIFF/EP, but all values below 6 are shifted down by one as well... |
Yes. They are similar and different. Don't change the API (keep print0xa217) and use the macro The nice thing about standards is there's plenty of choice! You also have to pay for them because it's the only source of revenue for most standards organisations. Have a nice weekend and thank you for your contributions. |
Handle more TIFF/EP vs Exif differences Moved translated values (GPS, Exif) closer to their respective lists
Let me know what you think about this last commit. I also took to opportunity to iron out some other minor TIFF/EP vs Exif differences. |
@kmilos Thank You Miloš for your work on this. For sure that code's in better shape today than it was last weekend. Thank You for working on this. I moved an orphaned declaration to be with his buddies. |
Thanks, looks good. Think we can merge and be done with Exif 2.32 support, otherwise we can keep prettifying the codebase till the cows come home... |
We move on, or we'll sink in the mud! |
See #929. Adding Exif 2.31 (2016) and Exif 2.32 support (2019).