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

Several Canon lenses not detected anymore with 0.27.3 that were detected by 0.27.2 #1420

Closed
webmeister opened this issue Dec 7, 2020 · 9 comments · May be fixed by #1421
Closed

Several Canon lenses not detected anymore with 0.27.3 that were detected by 0.27.2 #1420

webmeister opened this issue Dec 7, 2020 · 9 comments · May be fixed by #1421
Labels
lens Issue related to lens detection

Comments

@webmeister
Copy link
Contributor

Issue is similar to #1368. Lenses were correctly detected by exiv2 0.27.2, but detection broke with 0.27.3 due to 8859209 as mentioned in #1368 (comment).

368, "Sigma 18-35mm f/1.8 DC HSM | A" already got fixed in #1373. But looking at the changes made in 8859209, at least those other lenses were lost as well:

172, "Sigma 150-500mm f/5-6.3 APO DG OS HSM + 1.4x"
213, "Tamron SP 70-300mm f/4-5.6 Di VC USD"
624, "Sigma 14mm f/1.8 DG HSM | A"
624, "Sigma 150-600mm f/5-6.3 DG OS HSM | C"
624, "Sigma 150-600mm f/5-6.3 DG OS HSM | C + 1.4x"

Since I initially introduced support for 624 in cf4f14c, I know this ID to be correct. I can't say anything about the other two. You'll probably want to re-add all of them to keep backwards compatibility.

@webmeister webmeister added the bug label Dec 7, 2020
@clanmills clanmills added lens Issue related to lens detection and removed bug labels Dec 7, 2020
@clanmills
Copy link
Collaborator

I realise that it's quite a long read to see what happened with #1368. Lens recognition is a time sink. #1368 (comment)

If you want to see this fixed in C++ (instead of using the ~/.exiv2 mechanism) you are welcome to send a PR with test images and updates to the test suite. Lens recognition is guess work and the only way to ensure that subsequent changes to the lens recognition code don't break existing recognition/guesses is to have a test image and test in the test harness to reveal the regression.

webmeister added a commit to webmeister/exiv2 that referenced this issue Dec 7, 2020
Reintroduces lens IDs lost in 8859209. Fixes Exiv2#1420.
@webmeister
Copy link
Contributor Author

Tests are nice, but let's fix the simple regression first :)

@tester0077
Copy link
Collaborator

tester0077 commented Dec 7, 2020 via email

@clanmills
Copy link
Collaborator

Thanks, Arnold @tester0077

@webmeister I'm about to retire. I'm putting in 7x12+ hour days to write a book in which I have documented everything I know about both MetaData and Exiv2: https://clanmills.com/exiv2/book

@sridharb1 contributed the large change to the lens code to provide better compatibility with ExifTool. Because we don't have test images for every lens that can be recognised, we don't know when changes to the lens recognition code introduces regressions. Figuring out the change to re-enable your code is a "hit-n-miss" affair without a test image in the test suite. Your code in cf4f14c has regressed because you didn't supply an update to the test suite at that time.

Additionally, you have a work-around by using the file ~/.exiv2 to recognise you lens.

In order to finish the book this year, I'm avoiding making changes to the code. None-the-less, I spent several hours hours today on #1419 and #1395 and on the Exiv2 chat server to investigate things for @phako and @1div0.

I invite you to undertake the work that you wish me to do concerning your lens and allow me to get my book finished and let me. Please don't argue with me. A few words of appreciation would be more appropriate.

@webmeister
Copy link
Contributor Author

Why not accept Robin's suggestion and solution?

I assumed the removal of those lens IDs in 8859209 was a simple mistake and did not happen intentionally (the commit message says nothing about removing all lens IDs for which there are no tests). The solution for this mistake is to simply re-add the IDs, they worked before, they'll still work now. This is exactly what #1421 does.

I agree that having some tests to prevent such issues in the future would be nice. But that will definitely take longer than the five minutes it took to submit #1421, and as everyone's time (including mine) is limited, I'm not sure when that can be done. Therefore, at least fixing the regression for 0.27.4 now seemed more important. Adding new tests is more like adding a new feature, something you'd like to have as early as possible, but that can also be done later, since it does not break anyone's past use cases.

[As Robin] decided to move lens recognition functionality outside the compiled code would be the better long term solution, why not go with it? ;-)

I read the proposal in http://clanmills.com/exiv2/book/#future, but as far as I could tell that is just a proposal, there is no code yet that does it that way. Canon lenses are still recognised only by whatever canonmn_int.cpp does.

Apart from that, I like the idea of having only a single open lens database instead of multiple projects all trying to solve the same problem, but I'm not sure whether this can be achieved easily.

we don't know when changes to the lens recognition code introduces regressions

Well, in this case I was able to figure it out simply by reviewing the code, but of course automation is always better.

Additionally, you have a work-around by using the file ~/.exiv2 to recognise you lens.

Unfortunately, that won't work here, because it cannot distinguish between the three Sigma lenses that all share ID 624. And even if it did work, it would still be a regression for everyone else using exiv2, I'd still have to fix it manually on multiple machines, tell my friends how to fix it, etc. As a simple workaround for now, I downgraded to 0.27.2 and hope for 0.27.4 to fix this again.

I invite you to undertake the work that you wish me to do concerning your lens

I'll try to look into contributing a test to prevent such regressions in the future. I've got some ideas, but I haven't really looked at the existing test code yet, so I can't make any promises. And as you know, there are other things to keep us busy, you've got a book to write, I've got some ten thousand images to review ;)

@webmeister
Copy link
Contributor Author

clanmills closed this 2 minutes ago

Did you close this by mistake? As far as I can see, nothing changed in the code, so the issue is still present.

@clanmills
Copy link
Collaborator

No mistake. Please submit a PR.

@webmeister
Copy link
Contributor Author

Please submit a PR.

PR #1421 already exists, and all checks were successful, so it's ready for you to review/merge :) PR for test improvements will follow whenever I get the chance.

@clanmills clanmills linked a pull request Dec 8, 2020 that will close this issue
@webmeister
Copy link
Contributor Author

PR for test improvements will follow whenever I get the chance.

Test improvements are now available as #1428.

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 a pull request may close this issue.

3 participants