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

Test all Canon lenses #1428

Closed
wants to merge 7 commits into from

Conversation

webmeister
Copy link
Contributor

With this change, there is basic test coverage for all Canon lenses in canonCsLensType (src/canonmn_int.cpp), proving that all those lenses can in fact be detected by exiv2. Most importantly, this covers lenses without sample images, since all data necessary for detection is generated automatically, so no images need to be present.

Several issues have been found in the course of development and all of them are fixed as part of this PR, so that all tests pass. See individual commits 45e0995, ea4fa6c and c6083b8 for details.

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.
@clanmills
Copy link
Collaborator

@webmeister Thank you for undertaking this work. My retirement plan for Exiv2 was to release v0.27 by the end of December 2018, and then maintain the branch 0.27-maintenance for a couple of years with occasional "dot" releases which would include bug and security fixes.

The 'master' branch is a substantial change to Exiv2 with the modernisation of the code for C++XX (we hoped for C++17, the KDE people require C++11). As I never submit code to the 'master' branch, I have asked @piponazo to review and merge this.

If you wish to submit to the 0.27-maintenance branch, I will review your code.

@clanmills
Copy link
Collaborator

clanmills commented Dec 9, 2020

Another question about this, @webmeister I am assuming that you will accept my offer to take your code into the 0.27-maintenance branch and that sparks the question "is there an order dependency between #1421 and this PR?" I would prefer to submit this PR (and pass the test suite), then #1421 (with test suite changes). This would demonstrate how you have made the test suite more robust and it now detects the modified behaviour of #1421. Reasonable?

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
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"
There is no need to handle tests on Windows and Unix differently here.
Always using a shell allows for more flexibility when writing tests.
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.
@webmeister
Copy link
Contributor Author

Sounds good, new PR for 0.27-maintenance is #1429.

@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #1428 (d3075af) into master (38dc741) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1428   +/-   ##
=======================================
  Coverage   71.78%   71.78%           
=======================================
  Files         156      156           
  Lines       17389    17394    +5     
=======================================
+ Hits        12482    12487    +5     
  Misses       4907     4907           
Impacted Files Coverage Δ
src/canonmn_int.cpp 89.12% <100.00%> (+0.11%) ⬆️
src/tags_int.cpp 88.39% <100.00%> (+0.06%) ⬆️

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 38dc741...d3075af. Read the comment docs.

@clanmills
Copy link
Collaborator

@hassec. Thanks for commenting on this. @webmeister has done very good work on this and #1429. Would you two like to work together to bring this effort to a successful conclusion that requires as little maintenance as possible.

It's my birthday tomorrow (70 years young). C'mon: Make My Day with a beautiful and complete PR.

@webmeister
Copy link
Contributor Author

Other things kept me busy, but I'd be willing to commit a little more time to work on the feedback and get this merged.

Happy birthday to you, Robin :)

@clanmills
Copy link
Collaborator

Thanks, Guys. (@webmeister and @hassec). Deal with this when you're ready and we'll get this merged. I'm hope to finish my book by the end of February. Try to get the code submitted by "Code Complete" on 2021-02-28. In March, I'll get v0.27.4 ready and release RC1 on 2021-03-31. #1018 (comment)

I hope 2021 will be the year when Exiv2 moves forward without me pushing, pushing, pushing. Almost managed that with Dan and Luis in 2018 and then they changed jobs and life got in the way. One day I'll be free. The book explains everything. https://clanmills.com/exiv2/book/

Wonderful day yesterday. Facebook greetings from about 80 folks all over the world. The grandkids arrived in the evening to sing "Happy Birthday" on our drive. England is locked down. Depressing times. Life is good.

RobinBirthday

@clanmills
Copy link
Collaborator

Guys:

I've lost the plot here. This is a big change and I don't want to go through it lens-by-lens. Two comments:

  1. Can you submit the PR on the 0.27-maintenance branch and it'll ship in 0.27.4
    If it's in the 0.27-maintenance branch, it should be promoted in time to 'master'. It appears that nobody is working any more on 'master' which is worrying. Lots of good work has been put into 'master' which might be lost.

  2. @sridharb1 did a lot of work on the Canon lens for fix 1004 Canon Lens Fixes (0.27->master) #1157. Let's be careful not to "clobber" that with this change.

If @sridharb1, @webmeister and @hassec agree on a change to the 0.27-maintenance branch, I will accept it. I don't want to get sucked into studying this as my focus is on the book.

Base automatically changed from master to old-master February 28, 2021 05:57
@clanmills clanmills added this to the v1.00 milestone Apr 13, 2021
@clanmills
Copy link
Collaborator

@webmeister We're preparing to start a project on branch 'main' which will lead to Exiv2 v1.00 which is scheduled for 2021-12-15. We have a team of 8 contributors who are enthusiastic to participate. I would be very pleased for you to join the team and work on Canon lens detection.

@hassec
Copy link
Member

hassec commented Jun 27, 2021

included in #1692

@hassec hassec closed this Jun 27, 2021
@webmeister webmeister deleted the test-all-canon-lenses branch June 27, 2021 21:10
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants