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

fix646_NikonAF22 #900

Merged
merged 5 commits into from Jun 18, 2019
Merged

fix646_NikonAF22 #900

merged 5 commits into from Jun 18, 2019

Conversation

clanmills
Copy link
Collaborator

@clanmills clanmills commented Jun 7, 2019

There is a long discussion in #646 about this issue and my investigation into the how the makernotes are decoded. I will summarise here. Fix #646

History

The tag for Nikon's AutoFocus data is 0x00b7

Nikon encode their version of tag in the first 4 bytes. There was a 40 byte version of AutoFocus which decodes as Exif.NikonAf2.XXX. This new version (1.01) is 84 bytes in length and decoded as Exif.NikonAf22.XXX.

The two versions (NikonAF2 and NikonAF22) are now encoded as a set with the selector in tiffimage_int.cpp

    extern const ArraySet nikonAf2Set[] = {
        { nikonAf21Cfg, nikonAf21Def, EXV_COUNTOF(nikonAf21Def) },
        { nikonAf22Cfg, nikonAf22Def, EXV_COUNTOF(nikonAf22Def) },
    };

The binary layout of the record is defined in tiff image_int.cpp. For example, AF22 is:

    extern const ArrayCfg nikonAf22Cfg = {
        nikonAf22Id,      // Group for the elements
        littleEndian,     // Byte order
        ttUndefined,      // Type for array entry
        notEncrypted,     // Not encrypted
        false,            // No size element
        true,             // Write all tags
        true,            // Concatenate gaps
        { 0, ttUnsignedByte,  1 }
    };
    //! Nikon Auto Focus 22 binary array - definition
    extern const ArrayDef nikonAf22Def[] = {
        {  0, ttUndefined,     4 }, // Version
        {  4, ttUnsignedByte,  1 }, // ContrastDetectAF
        {  5, ttUnsignedByte,  1 }, // AFAreaMode
        {  6, ttUnsignedByte,  1 }, // PhaseDetectAF
        {  7, ttUnsignedByte,  1 }, // PrimaryAFPoint
        {  8, ttUnsignedByte,  7 }, // AFPointsUsed
        { 70, ttUnsignedShort, 1 }, // AFImageWidth
        { 72, ttUnsignedShort, 1 }, // AFImageHeight
        { 74, ttUnsignedShort, 1 }, // AFAreaXPosition
        { 76, ttUnsignedShort, 1 }, // AFAreaYPosition
        { 78, ttUnsignedShort, 1 }, // AFAreaWidth
        { 80, ttUnsignedShort, 1 }, // AFAreaHeight
    };

The two versions of the data are encoded in tiffimage_int.cpp

        { Tag::root, nikonAf21Id,      nikon3Id,         0x00b7    },
        { Tag::root, nikonAf22Id,      nikon3Id,         0x00b7    },

Binary Selector

The code to determine which version is decoded is in tiffimage_int.cpp

       {    0x00b7, nikon3Id,         EXV_COMPLEX_BINARY_ARRAY(nikonAf2Set, nikonAf2Selector) },

When the tiffvisitor encounters 0x00b7, he calls nikonAf2Selector() to return the index of the binary array to be used. By default it returns 0 (the existing nikonAf21Id). If the tag length is 84, he returns 1 for nikonAf21Id

    int nikonAf2Selector(uint16_t tag, const byte* /*pData*/, uint32_t size, TiffComponent* const /*pRoot*/)
    {
        int result = tag == 0x00b7 ? 0 : -1 ;
        if (result > -1 && size == 84 ) {
            result = 1;
        }
        return result;
    }

The decoder

EXV_CALL_MEMBER_FN(*this, decoderFct)(object);

This function understands how to decode byte-by-byte from const ArrayDef into the Exiv2 tag/values such as Exif.NikonAF22.AFAreaYPosition which it stores in the ExifData vector.

This is ingenious magic. I'll revisit/edit this explanation in the next few days when I have more time to explain this with more clarity.

Test files

_DSC8437.exv and test_issue_646.py Boiler plate. The .exv was extracted from a sample test files on Nikon's web site (location in #636)

@clanmills clanmills requested review from piponazo and D4N and removed request for piponazo Jun 7, 2019
@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #900 into 0.27-maintenance will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           0.27-maintenance     #900      +/-   ##
====================================================
+ Coverage             62.86%   62.88%   +0.01%     
====================================================
  Files                   156      156              
  Lines                 21598    21606       +8     
====================================================
+ Hits                  13578    13586       +8     
  Misses                 8020     8020
Impacted Files Coverage Δ
src/tiffvisitor_int.cpp 87.31% <ø> (ø) ⬆️
src/tiffimage_int.cpp 91.76% <ø> (ø) ⬆️
src/makernote_int.hpp 50% <ø> (ø) ⬆️
src/tags_int.cpp 86.41% <ø> (ø) ⬆️
src/tags_int.hpp 76.92% <ø> (ø) ⬆️
src/makernote_int.cpp 72.83% <100%> (+0.31%) ⬆️
src/nikonmn_int.cpp 58.73% <100%> (+0.21%) ⬆️

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 1eab4e0...db3a260. Read the comment docs.

@clanmills clanmills requested a review from piponazo Jun 7, 2019
@clanmills clanmills self-assigned this Jun 7, 2019
@clanmills clanmills added this to the v0.27.2 milestone Jun 7, 2019
Copy link
Collaborator

@piponazo piponazo left a comment

Thanks for the contribution!

Although I could not invest much time following the previous discussions, most of the changes look good to me. I just made a bunch of minor comments and questions.

src/nikonmn_int.cpp Outdated Show resolved Hide resolved
src/nikonmn_int.hpp Show resolved Hide resolved
src/tiffimage_int.cpp Show resolved Hide resolved
piponazo
piponazo previously approved these changes Jun 11, 2019
@mergify mergify bot dismissed piponazo’s stale review Jun 11, 2019

Pull request has been modified.

piponazo
piponazo previously approved these changes Jun 11, 2019
@mergify mergify bot dismissed piponazo’s stale review Jun 11, 2019

Pull request has been modified.

piponazo
piponazo previously approved these changes Jun 11, 2019
@piponazo
Copy link
Collaborator

piponazo commented Jun 11, 2019

@D4N do you want to take a look to the PR before we merge it ?

@clanmills
Copy link
Collaborator Author

clanmills commented Jun 11, 2019

Thanks for dealing with this, @piponazo . I've been working very hard yesterday and today on 582 (Andreas Sneider's request to support Sony FocusPosition), then I want to get Exiv2 0.27.2 RC1 done and released by Thursday before going to Normandy on Friday to run in the D-Day Half Marathon this weekend.

I'd like to discuss 882 FileIO::eof() calling stat with you a little more. I can't remember what I want to say to you about this, however it's on my TODO list. If I don't get that done this week, I will deal with it next week and we can decide if it's going in 0.27.2 or more-likely defer for 0.27.3.

Copy link
Member

@D4N D4N left a comment

Overall this looks good to me. I've added just a few comments

src/tiffvisitor_int.cpp Outdated Show resolved Hide resolved
src/nikonmn_int.cpp Outdated Show resolved Hide resolved
src/nikonmn_int.hpp Show resolved Hide resolved
@@ -106,7 +106,8 @@ namespace Exiv2 {
{ nikonWtId, "Makernote", "NikonWt", Nikon3MakerNote::tagListWt },
{ nikonIiId, "Makernote", "NikonIi", Nikon3MakerNote::tagListIi },
{ nikonAfId, "Makernote", "NikonAf", Nikon3MakerNote::tagListAf },
{ nikonAf2Id, "Makernote", "NikonAf2", Nikon3MakerNote::tagListAf2 },
{ nikonAf21Id, "Makernote", "NikonAf2", Nikon3MakerNote::tagListAf21 },
Copy link
Member

@D4N D4N Jun 11, 2019

Choose a reason for hiding this comment

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

[Suggestion] For consistency I'd suggest to call list & enumeration element *Af2 instead of *Af21.

Copy link
Collaborator Author

@clanmills clanmills Jun 11, 2019

Choose a reason for hiding this comment

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

You're right about this. We have two things which are very similar nikonAf2ID (AutoFocus version 1.00) and nikonAf22Id (AutoFocus version 1.01).

Copy link
Collaborator Author

@clanmills clanmills Jun 11, 2019

Choose a reason for hiding this comment

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

Dan: As always you are very sharp eyed. When I developed the code, I changed nikonAf2Id to nikonAf21Id and cut'n'pasted/morphed nikonAf21Id into nikonAf22Id. Later I discovered this broke the test suite, because there's a test that uses Exif.NikonAf2.something. I hacked that table to pass the test suite. I've restored nikonAf2Id.

It's really hard to understand the tiff parsing code. It's very abstract. I've been trying for 2 days to fix #582 and haven't got it work yet. I'll defer #582 until v0.27.3.

@D4N D4N added the to-master label Jun 11, 2019
@mergify mergify bot dismissed piponazo’s stale review Jun 11, 2019

Pull request has been modified.

piponazo
piponazo previously approved these changes Jun 18, 2019
@mergify mergify bot dismissed piponazo’s stale review Jun 18, 2019

Pull request has been modified.

@piponazo piponazo merged commit ef598ad into 0.27-maintenance Jun 18, 2019
8 checks passed
@mergify mergify bot mentioned this pull request Jun 18, 2019
@mergify mergify bot deleted the fix646_NikonAF22 branch Jun 18, 2019
piponazo added a commit that referenced this issue Jun 19, 2019
@mallman
Copy link
Collaborator

mallman commented Jul 4, 2019

As the originator of the request for and a beneficiary of this functionality, I want to offer my (long overdue) and effusive thanks to @clanmills and collaborators on developing and merging this PR.

I'm working to test support on my end, building 0.27.2-RC2, but having some problems with the build. I'll open an issue if I can't resolve them myself.

@mallman
Copy link
Collaborator

mallman commented Jul 4, 2019

I'm working to test support on my end, building 0.27.2-RC2, but having some problems with the build. I'll open an issue if I can't resolve them myself.

First of all, I was able to build successfully by updating my version of Cmake from 3.13.2 to 3.14.5. Seems like such a minor update, but it worked.

Second, I've verified that the exifprint command is producing what appear to be correct values for the Exif.NikonAf22.* keys when I run against my D850 NEF files.

Many thanks again!

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