-
Notifications
You must be signed in to change notification settings - Fork 282
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 Canon Lens Identification + Automatic Test of all Lenses #1692
Conversation
Generates a test case for every known lens from canonCsLensType, that first sets the corresponding lens metadata and then verifies that exiv2 maps it to the expected lens description. Only metadata fields that are relevant for lens identification are modified.
There is no need to handle tests on Windows and Unix differently here. Always using a shell allows for more flexibility when writing tests. (rebased by hassec)
Lenses that have the exact same ID, focal length and aperture as some other lens that comes earlier in the list (and thus always wins): * 137, "Tamron SP 17-50mm f/2.8 XR Di II VC" * 137, "Tamron SP 24-70mm f/2.8 Di VC USD" * 161, "Sigma 28-70mm f/2.8 EX" * 173, "Sigma 180mm EX HSM Macro f/3.5" * 180, "Zeiss Milvus 50mm f/1.4" * 183, "Sigma 150-600mm f/5-6.3 DG OS HSM | S" * 254, "Tamron SP 90mm f/2.8 Di VC USD Macro 1:1 F004" * 254, "Tamron SP 90mm f/2.8 Di VC USD Macro 1:1 F017" Lenses that share their IDs with other lenses, but have no or an unsupported focal length: * 33, "Voigtlander or Carl Zeiss Lens" * 131, "Sigma 4.5mm f/2.8 EX DC HSM Circular Fisheye"
Fixes some small inconsistencies, so that all lenses use the same format, that is also shared with other lens databases such as lensfun: * Always prefix aperture with f/ * Never add .0 to aperture * Always add mm to focal length * Always use | A for Sigma Art lenses
The mathematical calculation of fnumbers does not always match the expected values: For example for f/3.5 the precise mathematical value is 3.564..., which gets rounded to 3.6. Fix this special case by returning a value closer to the expected value.
When searching for the Tamron lens, only the string "300mm" is searched in the lens description, which also happens to be present for the Canon lens. Since the Canon lens comes first in the list, it wins. Fix this issue by prefixing the search string with a single space so it always has to match the full focal length specification.
Lenses with and without a TC may share the same lens ID. Prefer entries that explicitly mention the TC.
This commit does some restructuring to make common utils available for future similar test for other brands
For each lens, its test target is now defined as the list of all lenses which are possible given that lenses exif values.
If multiple choices are possible they are now all reported. This behaviour is now the same as it is in exiftool. All lenses are tested in the new test_canon_lenses.py test
Codecov Report
@@ Coverage Diff @@
## main #1692 +/- ##
==========================================
- Coverage 66.90% 66.85% -0.05%
==========================================
Files 151 151
Lines 20762 20729 -33
==========================================
- Hits 13890 13858 -32
+ Misses 6872 6871 -1
Continue to review full report at Codecov.
|
Great, thanks for taking over :)
They are still reported as a single string, but connected with |
@webmeister, 100% correct, one large string with My original motivation was for darktable to be able to warn the user that it can't determine the lens correctly, and make the user aware of this situation. I plan to ping them again once the feature is there, or maybe even dable myself a bit in the darktable 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.
looks fine to me
This PR includes 2 features:
canonmn_int.cpp
I've chosen to include both of these features in one PR, because I first created the tests such that they produce the output that I was hoping to obtain, and then followed that by implementing the corresponding features in exiv2.
The test functionality in this PR is a continuation of #1428, so thanks to @webmeister for the idea and providing a nice starting point :)
Identification of Lenses now uses all the available info we have in a canon maker note:
Various lens descriptions were adjusted such that they are now all correctly reported.
In case it isn't possible to narrow it down to only a single possible lens, all possible lenses are reported. (you can see examples of that in the diff)
This is the same behavior people know from exiftool.