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 Canon Lens Identification + Automatic Test of all Lenses #1692

Merged
merged 17 commits into from Jun 20, 2021

Conversation

hassec
Copy link
Member

@hassec hassec commented Jun 8, 2021

This PR includes 2 features:

  • New procedure to determine the correct lens for canon maker note
  • automatic tests of all lenses specified in 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:

  • max and min focal length
  • max aperture at the focal length the picture was taken at
  • teleconverter factor which is applied to the above

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.

webmeister and others added 15 commits June 2, 2021 21:21
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
@hassec hassec added lens Issue related to lens detection testing Anything related to the tests and their infrastructure makerNote Anything related to one of the various supported MakerNote formats labels Jun 8, 2021
@hassec hassec added this to the v1.00 milestone Jun 8, 2021
@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #1692 (efb0af8) into main (f30022d) will decrease coverage by 0.04%.
The diff coverage is 88.63%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/canonmn_int.cpp 70.21% <87.50%> (-3.02%) ⬇️
src/tags_int.cpp 87.17% <100.00%> (+0.10%) ⬆️
src/minoltamn_int.cpp 53.50% <0.00%> (+0.43%) ⬆️

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 f30022d...efb0af8. Read the comment docs.

@webmeister
Copy link
Contributor

Great, thanks for taking over :)

In case it isn't possible to narrow it down to only a single possible lens, all possible lenses are reported.

They are still reported as a single string, but connected with *OR*, right? Are there already consumers of exiv2 that can take advantage of this feature or do we need to make them aware of it first? For example, I could imagine importing an image into darktable and then being asked to choose one of the lenses when it detects the presence of *OR*.

@hassec
Copy link
Member Author

hassec commented Jun 9, 2021

@webmeister, 100% correct, one large string with *OR* as divider.

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.
Connected to that, we had a discussion about this topic in the exvi2 matrix chat that has a few darktable devs.

I plan to ping them again once the feature is there, or maybe even dable myself a bit in the darktable code 😅
But in any case, this will only come once exiv2 v.1.0 is shipped and darktable then adapts to it.
So still a bit of a wait, but the long term goal is certainly there 😉

Copy link
Member

@alexvanderberkel alexvanderberkel left a 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

@hassec hassec merged commit 2f83b7e into main Jun 20, 2021
@mergify mergify bot deleted the hassec_canon_lens_test branch June 20, 2021 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lens Issue related to lens detection makerNote Anything related to one of the various supported MakerNote formats testing Anything related to the tests and their infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants