Implement Nikon LensData version 8 handler (0.27->master) #1437
Conversation
There was a problem hiding this comment.
This is an outstanding PR. By far the best PR I've seen in 12 years of working on Exiv2. One more PR like this and I'll give you a mention on page 2 of my book.
Dan, Luis and Kev have all contributed outstanding PRs for security, build and test. You are the only person who has ever updated NikonArrayIdx. I'll document that in my book.
Comments:
-
Lots of test suite exceptions on macOS to be investigated. It's odd that the CI is green and happy.
-
Apertures are "apex numbers" (as documented in the exiv2.1 man page).
617 rmills@rmillsmm-local:~/gnu/github/exiv2/NikonLensData800/build $ nm -g --demangle lib/libexiv2.dylib | grep fnumber
00000000001fadc0 T Exiv2::Internal::fnumber(float)
618 rmills@rmillsmm-local:~/gnu/github/exiv2/NikonLensData800/build $ The code is in tags_int.cpp#2490:
float fnumber(float apertureValue)
{
return std::exp(std::log(2.0f) * apertureValue / 2.f);
}-
In PR Test all Canon lenses (backport to 0.27-maintenance) #1429, the user reported rounding errors in the calculator. Regrettfully, the user didn't perform the requested changes in the review. He over-reached and modified the output from F4.5 to 4.5/f which has an impact on the test suite and man page. That would be OK for v0.28, however I don't think that's an appropriate change for 0.27-maintenance.
-
It's odd that Visual Studio 2013 is failing. It looks as though the builder refused to run. On the next update to the PR, this will magically work. I'll build it on my Windows VM and see what happens.
As you know, I want to retire. A PR like this makes me believe that the open-source model can work. Good Job!
We have some work to get this finished. Well worth the effort. Will you work with me to get this finished, or do you want me to do the work?
|
Hi @clanmills, I'm absolutely interested in following through with the PR. Just won't have too much time this week as I'm on vacation for a week and don't know if/how much time I might have to do this. |
|
We'll deal with this when you're ready. I'm very enthusiastic to give you encouragement. |
|
Good News about the macOS and Visual Studio 2013. I reran the test suite on macOS (without changing anything) and it passed. How odd that it moaned about lots of stuff this morning. For sure it passed on the CI, so let's forget about this. I've manually built with Visual Studio 2013 and it builds and passes the test suite. So, let's ignore the CI. C:\cygwin64\home\rmills\gnu\github\exiv2\NikonLensData800\build>cmake --build . --config Release --target python_tests
Microsoft (R) Build Engine version 12.0.40629.0
[Microsoft .NET Framework, version 4.0.30319.42000]
Copyright (C) Microsoft Corporation. All rights reserved.
---- Running python_tests ----
bash -c cd ../tests; python3 runner.py
........127.0.0.1 - - [26/Dec/2020 18:22:31] "GET / HTTP/1.1" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "HEAD /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "GET /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "HEAD /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "GET /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "HEAD /Reagan.tiff HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "GET /Reagan.tiff HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "HEAD /Reagan.tiff HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "GET /Reagan.tiff HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "HEAD /exiv2-bug922a.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "GET /exiv2-bug922a.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "HEAD /exiv2-bug922a.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:32] "GET /exiv2-bug922a.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:33] "HEAD /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:33] "GET /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:33] "HEAD /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:33] "GET /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:33] "HEAD /table.jpg HTTP/1.0" 200 -
127.0.0.1 - - [26/Dec/2020 18:22:33] "GET /table.jpg HTTP/1.0" 200 -
....................................................s................s............ss............s.......................................s...................................................s..........
----------------------------------------------------------------------
Skipped. Because exiv2 is not built with nls.
Ran 207 tests in 42.705s
OK (skipped=7)
Building Custom Rule C:/cygwin64/home/rmills/gnu/github/exiv2/NikonLensData800/CMakeLists.txt
C:\cygwin64\home\rmills\gnu\github\exiv2\NikonLensData800\build>bin\exiv2 -vV --grep version --grep date --grep time --grep exiv2
exiv2 0.27.4.9
exiv2=0.27.4
version=12.00 (2013/x64)
date=Dec 26 2020
time=18:10:50
processpath=C:\cygwin64\home\rmills\gnu\github\exiv2\NikonLensData800\build\bin
package_name=exiv2
executable=C:\cygwin64\home\rmills\gnu\github\exiv2\NikonLensData800\build\bin\exiv2.exe
library=C:\cygwin64\home\rmills\gnu\github\exiv2\NikonLensData800\build\bin\exiv2.dll
config_path=C:\Users\rmills\exiv2.ini
xmlns=mediapro:http://ns.iview-multimedia.com/mediapro/1.0/
C:\cygwin64\home\rmills\gnu\github\exiv2\NikonLensData800\build>I looked at Exiv2::Internal::fnumber(). That's not called by the NikonLd code (about which I know almost nothing). Let's make a little effort to investigate the pedigree of NikonLd and compare with ExifTool. We should also investigate the rounding error reported in #1429. |
clanmills
changed the title
clanmills
changed the title
|
I'm going to proceed to merge this. I'll update the test suite (as discussed below) in another PR. I'm not a photographer, so I don't really know what an Apex number is all about. It was discussed a few years ago: https://dev.exiv2.org/issues/1039#note-8 For sure, I see different code. In nikonmn_int.cpp, we have: std::ostream& Nikon3MakerNote::printAperture(std::ostream& os,
const Value& value,
const ExifData*)
{
std::ios::fmtflags f( os.flags() );
if (value.count() != 1 || value.typeId() != unsignedByte) {
os << "(" << value << ")";
os.flags(f);
return os;
}
double aperture = pow(2.0, value.toLong()/24.0);
std::ostringstream oss;
oss.copyfmt(os);
os << std::fixed << std::setprecision(1) << "F" << aperture;
os.copyfmt(oss);
os.flags(f);
return os;
}And in tags_int.cpp#2490: float fnumber(float apertureValue)
{
return std::exp(std::log(2.0f) * apertureValue / 2.f);
}For sure the ~/.exiv2 lookup is good: And I've extracted the .exv and that's also working. So, I'll add that to the test suite: The output from exiftool and exiv2 are a little different: $ exiftool ~/temp/CH0_0174.exv | grep -i lens
Lens Type : G VR
Lens : 70-200mm f/2.8
Lens Data Version : 0801
Lens ID Number : 119
Lens F Stops : 6.00
Lens Info : 70-200mm f/2.8
Lens Make :
Lens Model :
Lens Serial Number :
Lens ID : AF-S VR Zoom-Nikkor 70-200mm f/2.8G IF-ED
Lens Spec : 70-200mm f/2.8 G VR
$ bin/exiv2 -g lens/i ~/temp/CH0_0174.exv
Exif.Nikon3.LensType Byte 1 D G VR
Exif.Nikon3.Lens Rational 4 70-200mm F2.8
Exif.Nikon3.LensFStops Undefined 4 6
Exif.NikonLd4.LensIDNumber Byte 1 Nikon AF-S VR Zoom-Nikkor 70-200mm f/2.8G IF-ED
Exif.NikonLd4.LensFStops Byte 1 F6.0
Exif.NikonLd4.LensID Short 1 0
Exif.Photo.LensSpecification Rational 4 700/10 2000/10 280/100 280/100
Exif.Photo.LensMake Ascii 6
Exif.Photo.LensModel Ascii 65
Exif.Photo.LensSerialNumber Ascii 11
$ @hassec Please test this with darktable/lensfun. If the differences between exiftool and exiv2 are causing difficulty, I know you're easily smart enough to fix that and submit another PR. |
|
Another comment about this concerning Aperture. exif tool and exiv2 agree 100% about this: I'm not convinced about Exif.NikonLd4.MaxAperture: Anyway. This is a good step in the right direction. It's merged. I'll submit a PR with an update to the test suite and get back to finishing my book. |
|
Ok getting back into this now :)
|
|
Happy New Year. Good to have you back here. Because I'm not a photographer, I don't know what a Z-Mount is. I have a D5300 with two lenses (a sigma 18-250 for outdoor shots and a Nikon 12-24 for indoor). The decoding of the makernote takes place in the TiffVisitor code. The Ld4 structure is used to decode tag 0x0098 (LensData) in the MakerNote. If Exiv2 "manufactures" data that's irrelevant for an F-Mount, it's almost certainly harmless. If you're not complaining, neither am I, so let's leave it be. If this causes somebody unhappiness, they'll raise an issue which can be investigated when we understand the evil consequences to be addressed. Since we spoke before the holidays, I've agree to do one more release. The darktable community have made a huge fuss about ISOBMFF (for .CR3, .AVIF and .HEIC) support. It was to be included in Exiv2 v0.27.3 and two Exiv2 people red-flagged it as a potential patent infringement. I researched ISOBMFF and implemented it in tvisitor.cpp in my book. An Engineer in the Czech Republic is working with me to port the code from the book to Exiv2. Alex Esseling in Germany has agree to set up a zoom meeting with the darktable people. Would you be willing to attend? I'm also going to invite @LeoHsiao1 who has do great work on the Exiv2 test suite and @kmilos who has done great work on DNG and TIFF-EP support. However, I'm not dumping on anybody. The purpose of the meeting is to discuss how, who, when this can be done. 10+ will attend the meeting and 7+ will agree to nothing! In total madness, I have agreed to another 200 hour project to release v0.27.4 with ISOBMFF Support. The book will be finished and I really will be allowed to retire. |
|
Ah, sorry about that. The Z-Mount is the new lens mount used by Nikon's mirrorless cameras like the Z6 and Z7. But Nikon also has an F-mount to Z-mount adapter (normally called FTZ adapter) which enables the use of Nikon F-mount lenses (like yours) on these mirrorless systems. And the new LensData version for these new mirrorless cameras has fields which are set depending on which lens one uses. And re the zoom meeting, sure I'm happy to attend and listen in. Not sure how much I'll be able to contribute but always happy to learn :) |
|
Right. Thanks for the explanation about the lens mount. Maybe when I retire, I'll join the local photography club and learn who to use a camera and PhotoShop. Thanks for being willing to join the meeting with darktable. I don't know if it will happen. Alex offered to help out, but what he really intended was to push me into more work. I've asked him to set up the meeting. I don't expect you to get involved with ISOBMFF as Peter has accepted the challenge. This PR is already merged, so I think we're done. Thank You for working on this. |
|
For the record, it looks like we need similar bumps for |
This is related to #1433.
I've had some time to dive a bit into the exiv2 code and found that the decryption algorithm is actually already implemented in the function
ncryptinmakernote_int.cppThe reason that the
Exif.Nikon3.LensDatawasn't encoded is that the new NikonLensDataversion of8.0and8.01weren't known yet.So after some use of GDB and "when in doubt -> cout", I managed to implement the basics and can now decode the new
LensDataversion. I've tested it with a couple of files and seem to be able to get the same output as theexiftoolproduces.This isn't quite ready yet, but I'm opening the PR to get some review going rather earlier than later as there are some open questions.
The layout of the new
LensDatais based on what is provided here by exiftoolIt seems that fields up to index 20 are pretty much the same as what is defined as
nikonLd3in exiv2.These fields are used if an F-mount lens is mounted via a FTZ adapter.
If using a native Z Mount lens these fields are all null and only the last 4 fields are used.
The value conversions for these fields are also taken from
exiftool(see here)Open questions:
exiftoolchecks if all the first fields are null and based on this knowledge conditionally only prints the F or Z mount relevant fields. I think this makes sense as the other fields are just gibberish when converted to values. Any suggestions how this could/should be done?LensIDNumberfor an F and Z mount lens, so should a new section be introduced for that? MaybenikonZ? Or we could prefix the number withZbefore searching thenikonsection. Then one can specify and entry for a Z mount lens simply byZ-12=my z mount lens nameToDo: