Fix regression in Canon lens detection

[webmeister]

Reintroduces lens IDs lost in 8859209. Fixes #1420.

[clanmills]

I require test images and update to the test suite to accept changes in lens recognition.

[webmeister]

To fix a simple regression, really? This is not adding support for new lenses, it is just restoring functionality that was present before and probably got damaged by accident. @sridharb1 please correct me if this assumption is wrong and you had a specific reason to remove support for those lenses in #1004.

[clanmills]

Yes. I need test files and changes to the test suite. I we don't do this now, it's only a matter of time until @sridharb1 (or another user) submits a load of changes and breaks your "regression fix". And history will repeat itself.

If you provide test changes now, we will know the this future change from @sridharb1 has broken your fix.

I wish Exiv2 had never become involved in Lens recognition. The lens is not stored in the metadata. It's all a guess.

I won't be pursuing my thoughts/proposals about M2Lscript. It'll be too much work. I'm exhausted and burnt out by the open-source community.

[webmeister]

And history will repeat itself.

I see what you mean. I'd still appreciate it if this PR can be merged in time for the next exiv2 release, so that there is only one version where detection of those lenses is broken, not more than one. But if tests are a precondition for this to be merged, I'm not sure this will work, I don't even have images for all those lenses. (I do have some ideas for a proper regression test that works without actual images, but I'm not that familiar with your code base beyond canonmn_int.cpp, so this might take some time.)

So, from my point of view, merging this PR as is does not make everything perfect, but at least it improves the current state.

[clanmills]

Ah, you are very persuasive. I'm going to pass on this for now. It is passing the CI, so it can be merged.

https://discuss.pixls.us/t/maintainer-urgently-needed-for-exiv2/21547/8

I intend to retire in 2 weeks with the book complete. The 0.27-maintenance will be 100% closed (with 100+ matters closed since v0.27.3). No known security issues, and no CVEs reported in 2020, and a 200 page book (of text, drawings and code) discussing everything I know about Exiv2. I can't think of anything else to be done to prepare Exiv2 for the future. https://clanmills.com/exiv2/book.

[webmeister]

Thank you very much, @clanmills.

This means that exiftool most likely also has a problem.

Correct, exiftool does not recognize those lenses either. But I did not really care about exiftool in the past, so I did not submit a patch there.

[sridharb1]

To fix a simple regression, really? This is not adding support for new lenses, it is just restoring functionality that was present before and probably got damaged by accident. @sridharb1 please correct me if this assumption is wrong and you had a specific reason to remove support for those lenses in #1004.

Yes, this assumption is wrong. The specific reason is that now, the canonmn_int.cpp matches with exiftool (or at least it did when I submitted the change).

Specifically, you can see that some of the lenses that you are adding are already there in the list, but in different offsets. And some of them don't exist in exiftool anymore (like the 1.4x and 2x versions of the 150-500). Please review the file again and see if these additions are required. Please see what would happen if there are multiple entries in the list. If the lens metadata still reports the old offsets and they are not being recognized, then I don't have a problem with the change, please go ahead.

[clanmills]

Thanks, @sridharb1 Good to hear from you. Hope you and yours are all safe and well. All's fine with the Mills family in England.

Thank you for speaking about this issue. I believe you are supporting my position that we should add test images (and test code) to justify/support every change in the lens recognition code.

Maybe somebody one day will use the test images available from ExifTool, pixls.us and on my MacMini to strengthen every aspect of testing including comparing Metadata recognised by ExifTool and Exiv2.

[sridharb1]

Thanks, @sridharb1 Good to hear from you. Hope you and yours are all safe and well. All's fine with the Mills family in England.

Yes, doing well and good to hear that the "midnight oil" is being burned to get the book out. I look forward to it.

Thank you for speaking about this issue. I believe you are supporting my position that we should add test images (and test code) to justify/support every change in the lens recognition code.

Yes.

[webmeister]

And some of them don't exist in exiftool anymore

I'd guess those never existed in exiftool in the first place. So when syncing with exiftool, only new lenses should be added, but none should be removed.

Please see what would happen if there are multiple entries in the list.

That works fine, some lenses report different IDs depending on their firmware version.

[clanmills]

I've searched my 16,466 test files for an image that uses any of these three lenses. Nope. None of them.

.../testfiles $ find . -print0 -type f | xargs -0 ls -l | wc
  16466  137831 1172245
.../testfiles $ find . -print0 -type f | xargs -0 exiv2 -pv --grep Make/i 2>/dev/null | wc
  12202 62517066 202021453
.../testfiles $ find . -print0 -type f | xargs -0 exiv2 -pv --grep Make/i 2>/dev/null | grep Canon | wc
   2514 1678857 4000459
.../testfiles $ find . -print0 -type f | xargs -0 exiv2 -pv --grep lens/i 2>/dev/null | grep Canon | wc
   1259   10277  139140
.../testfiles $ find . -print0 -type f | xargs -0 exiv2 -pv --grep lens/i 2>/dev/null | grep Canon | grep -e 172 -e 213 -e 624
1586 rmills@rmillsmm-local:/Users/Shared/Jenkins/Home/userContent/testfiles $ 

Just for grins, I've searched the back up of my Photo Library of 84,000 photos. 1340 with a Canon+lens information (a digital rebel I had 2003-2006). Again, none lens with id = 172 | 213 | 624.

.../Backup/Photos $ find . -print0 -type f | xargs -0 ls -1 | wc 
 83901  253088 4492002
.../Backup/Photos $ find . -print0 -type f | xargs -0 exiv2 -pv --grep lens/i 2>/dev/null | grep Canon | wc
   1340   12470  148087
.../Backup/Photos $ find . -print0 -type f | xargs -0 exiv2 -pv --grep lens/i 2>/dev/null | grep Canon | grep -e 172 -e 213 -e 624
1592 rmills@rmillsmm-local:/Volumes/Backup/Photos $ 

We can "cook one up a test image" using something like $ exiv2 -M'set Exif.CanonXX.LensId 172' foo.jpg. Should get an image that uses a fairly similar lens and camera and then modify the LensID (or whatever Canon calls the field). It is do-able. A little patience and effort and it can be done. Better than no test image and you returning in two years to moan about this again. Of course, I won't be here. However, I'm trying to save your future time and frustration.

[webmeister]

We can "cook one up a test image" using something like $ exiv2 -M'set Exif.CanonXX.LensId 172' foo.jpg.

This is basically the idea I had in mind, just that I'd like to have a for loop over canonCsLensType, to do that with every lens currently supported, not just those few I've touched here. The fake data (for focal length, max aperture, etc.) can be easily generated from the lens description. Then such a test can verify not only that no lenses are dropped accidentially, but also that all the logic behind lensIdFct continues to work correctly.

[clanmills]

We have to be careful here. Lens recognition involves perverse logic involving other metadata such as Min/Max Aperture, Min/Max Focal Range. Faking every lens is neither possible nor desirable. Let's restrict this to your lens. How difficult can it be? You must have images otherwise how did you know that the Sridhar's changes clobbered your lens?

Notice that none of this is "real/solid/data in the file". It's all about interpreting data in the file to guess the Lens.

It's been another long day with distractions into #1422, #1423, #1424 and discussion on the chat server. Please let me focus on my book for a couple of days and we can talk about this later in the week.

[webmeister]

Faking every lens is neither possible

Except for things like 33, "Voigtlander or Carl Zeiss Lens" it should be possible for pretty much all of them, because that is how the algorithm works: to distinguish between multiple lenses sharing the same ID, it tries to find this extra information (focal length, aperture) within the description string, so a test can also work the other way round and generate the data from the string.

Thinking about it, 33, "Voigtlander or Carl Zeiss Lens" can probably be removed from the list, because the algorithm would never pick it anyway (it does not mention the aperture at all). So, it should indeed be possible to have at least a basic test for every lens using this approach.

Please let me focus on my book for a couple of days and we can talk about this later in the week.

Sure :)

[clanmills]

I asked you yesterday:

Please don't argue with me. A few words of appreciation would be more appropriate.

You have argued and provided no words of appreciation. I want to finish the book in 2020 and believe that's more important than further discussion of this PR. It's up to a future maintainer/manager to deal with this.

[webmeister]

that's more important than further discussion of this PR

In the beginning you said:

It is passing the CI, so it can be merged.

So I expected it to be merged, and did not discuss it further. Everything else addressed to you was about improving the tests in general, which has now lead to a separate PR, see #1428.

[clanmills]

@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.