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

New PR to replace fix981_canonAutoFocus2 which will be closed. #988

Merged
merged 1 commit into from Sep 15, 2019

Conversation

clanmills
Copy link
Collaborator

@clanmills clanmills commented Aug 23, 2019

This PR replaces fix981_canonAutoFocus2 (with the same code). This has been done because there was a long discussion about fix981_canonAutoFocus2. For easier team review, there is no need to read about 30 discussion items in fix981_canonAutoFocus2

fix981_canonAutoFocus2 was a replacement for fix981_canonAf which was abandoned as it revealed architectural limits in the binary tag decoders.

This PR (and fix981_canonAutoFocus2) use a decoder listed in TiffMappingInfo to decode Exif.Canon.AFInfo. The decoding function "manufactures" Exif tags such as Exif.Canon.AFNumPoints from the data in Exif.Canon.AFInfo. These tags must never be written to file and are removed from the metadata in exif.cpp/ExifParser::encode().

Three of the tags created (AFPointsInFocus,AFPointsSelected, AFPrimaryPoint) are bitmasks. As the camera can have up to 64 focus points, the tags are a 64 bit mask to say which points are active. The function printBitmask() reports data such as 1,2,3 or (none).

This decoding function decodeCanonAFInfo() added to TiffMappingInfo manufactures the new tags. Normally, tags are processed by the binary tag decoder and that approach was taken in branch fix981_canonAf. However, the binary tag decoder cannot deal with AFInfo because the size of some metadata arrays cannot be determined at compile time.

We should support decoding AFInfo in 0.28, however we should NOT auto-port this PR. We can avoid having to explicitly delete tags from the metadata before writing by adding a "read-only" flag to TagInfo. This would break the Exiv2 v0.27 API and has been avoided. There is an array in decodeCanonAFInfo() which lists the "manufactured" tags such as Exif.Canon.AFNumPoints. In the Exiv2 v0.28 architecture, a way might be designed to generate that data at run-time.

It has been interesting to work on this issue. I've learnt more about the Exiv2 architecture for tag and MakerNote decoding and these lessons will be documented in the book being written for #911 .

@clanmills clanmills requested a review from piponazo Aug 23, 2019
@clanmills clanmills mentioned this pull request Aug 23, 2019
@clanmills clanmills self-assigned this Aug 23, 2019
@clanmills clanmills added this to the v0.27.3 milestone Aug 23, 2019
@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #988 into 0.27-maintenance will increase coverage by 0.09%.
The diff coverage is 97.67%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           0.27-maintenance     #988      +/-   ##
====================================================
+ Coverage             62.94%   63.04%   +0.09%     
====================================================
  Files                   157      157              
  Lines                 21618    21661      +43     
====================================================
+ Hits                  13608    13656      +48     
+ Misses                 8010     8005       -5
Impacted Files Coverage Δ
src/canonmn_int.cpp 87.5% <ø> (ø) ⬆️
src/exif.cpp 80.71% <ø> (ø) ⬆️
src/tiffvisitor_int.hpp 100% <ø> (ø) ⬆️
src/tags_int.hpp 76.92% <ø> (ø) ⬆️
src/tiffimage_int.cpp 91.76% <ø> (ø) ⬆️
src/tiffvisitor_int.cpp 87.71% <100%> (+0.5%) ⬆️
src/tags_int.cpp 87.59% <92.3%> (+1.18%) ⬆️
src/tags.cpp 57.96% <0%> (+1.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db70528...90f9f0b. Read the comment docs.

Copy link
Collaborator

@derselbst derselbst left a comment

I gave this a final test and found one minor issue: Exif.Canon.AFInfoSize should be added to the filtered tags as well. Apart from that it works absolutely fine. Thank you very much for your effort, Robin!

@clanmills
Copy link
Collaborator Author

clanmills commented Aug 23, 2019

Good Catch, Tom. I missed that. I cut'n'pasted those keys from another file and dropped one. Thanks for spotting and reporting that.

I'm not a photographer in the sense of knowing how about cameras really work. For my Nikon D5300, in manual focus mode, there is a red rectangle which you can move about with the arrow keys. Presumably the "PrimaryPoint" bitmask tells you which points lie inside.

From my school-boy Physics, I thought a lens has a focal length and the aperture determines the depth of field. So, my guess is that the camera selects a point as its "Primary Focus". From radar (or ultra-sound), it knows the points in the image that seem to be in focus and records that in Exif.Canon.AFPointsInFocus.

My neighbour is a member of the local photography club. Maybe they'll ask me to give a talk about Exiv2 and I could ask them to explain some of these puzzles.

Copy link
Member

@D4N D4N left a comment

Overall this looks good, I've flagged a few potential issues.

src/tags_int.cpp Show resolved Hide resolved
src/tags_int.cpp Outdated Show resolved Hide resolved
tests/bugfixes/github/test_issue_981.py Outdated Show resolved Hide resolved
src/tags_int.hpp Outdated Show resolved Hide resolved
tests/bugfixes/github/test_issue_981.py Outdated Show resolved Hide resolved
tests/bugfixes/github/test_issue_981.py Outdated Show resolved Hide resolved
src/tiffvisitor_int.cpp Outdated Show resolved Hide resolved
src/tiffvisitor_int.cpp Outdated Show resolved Hide resolved
src/tiffvisitor_int.cpp Outdated Show resolved Hide resolved
};
// check we have enough data!
uint16_t count = 0;
for ( uint16_t i = 0; records[i].tag != 0xffff ; i++) count += records[i].size ;
Copy link
Member

@D4N D4N Aug 27, 2019

Choose a reason for hiding this comment

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

Isn't this equivalent to:

Suggested change
for ( uint16_t i = 0; records[i].tag != 0xffff ; i++) count += records[i].size ;
const uint16_t count = 8 + 4 * nPoints + 3 * nMasks;

Copy link
Collaborator Author

@clanmills clanmills Aug 29, 2019

Choose a reason for hiding this comment

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

Well yes, and no. It's not static. At the moment it's 4 records of nPoints and 3 of nMasks. However In development, we've modified the number of records with nPoints and nMasks in the records[] array more than once. It's safer to count how much data we need, and be certain it's available.

This PR (and fix981_canonAutoFocus2) use a decoder listed in TiffMappingInfo to
decode Exif.Canon.AFInfo. The decoding function "manufactures" Exif tags such as
Exif.Canon.AFNumPoints from the data in Exif.Canon.AFInfo. These tags must never
be written to file and are removed from the metadata in
exif.cpp/ExifParser::encode().

Three of the tags created (AFPointsInFocus,AFPointsSelected, AFPrimaryPoint) are
bitmasks. As the camera can have up to 64 focus points, the tags are a 64 bit
mask to say which points are active. The function printBitmask() reports data
such as 1,2,3 or (none).

This decoding function decodeCanonAFInfo() added to TiffMappingInfo manufactures
the new tags. Normally, tags are processed by the binary tag decoder and that
approach was taken in branch fix981_canonAf. However, the binary tag decoder
cannot deal with AFInfo because the size of some metadata arrays cannot be
determined at compile time.
D4N
D4N previously approved these changes Sep 14, 2019
@mergify mergify bot dismissed D4N’s stale review Sep 14, 2019

Pull request has been modified.

D4N
D4N approved these changes Sep 14, 2019
Copy link
Member

@D4N D4N left a comment

@clanmills Thank you for addressing my comments! I've squashed the commits and rebased them on 0.27-maintenance and would merge this once the CI is green again.

@derselbst
Copy link
Collaborator

derselbst commented Sep 14, 2019

Pls. consider to rename AFPrimaryPoints based on my research, before merging this:

#981 (comment) :

[...] unusable AF points are reported as AFPrimaryPoints.

Perhaps it would be worth to rename that field to something like AFPointsUnusable or AFPointsDisabled.

@clanmills
Copy link
Collaborator Author

clanmills commented Sep 14, 2019

@derselbst I thought we adopted "Primary" to align with the Exiftool documentation. https://sno.phy.queensu.ca/~phil/exiftool/TagNames/Canon.html

I'm OK with changing this, however perhaps you could discuss with Phil. Would it be OK to accept things as they stand and I can get Exiv2 v0.27.3 finished. We can change the name as a separate issue in v0.27.4 if ExifTool adopts your suggestion. OK?

I hope to get Exiv2 v0.27.3 RC1 released early next week and (if nothing bad happens), that code will be released as v0.27.3 on 2019-09-30 (on schedule).

Exiv2 v0.27.4 is scheduled for 2019-12-31.

@derselbst
Copy link
Collaborator

derselbst commented Sep 15, 2019

I just wanted to make sure that we

  • don't forget about this, and
  • don't unnecessarily break API between releases by changing names.

On the other hand, I don't mind if you guys want to postpone renaming to the next maintenance release. In this case, I would just open a new issue to discuss this.

@clanmills
Copy link
Collaborator Author

clanmills commented Sep 15, 2019

I don't believe the name is revealed via the API. I think we should open a new issue to deal with your concern and I will ask Phil Harvey to comment.

@clanmills clanmills merged commit 961409a into 0.27-maintenance Sep 15, 2019
8 checks passed
@mergify mergify bot deleted the fix981 branch Sep 15, 2019
piponazo added a commit that referenced this issue Sep 17, 2019
New PR to replace fix981_canonAutoFocus2 which will be closed. (bp #988)
mergify bot pushed a commit that referenced this issue Oct 1, 2019
piponazo pushed a commit that referenced this issue Dec 10, 2019
piponazo pushed a commit that referenced this issue Dec 11, 2019
@clanmills clanmills mentioned this pull request Apr 27, 2020
1div0 pushed a commit to 1div0/exiv2 that referenced this issue Jun 7, 2020
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.

None yet

4 participants