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.

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

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.

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:https://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 ;)

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.

No mistake. Please submit a PR.

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.

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

Test improvements are now available as #1428.