Fix_1368 Sigma 18-35mm f/1.8 DC HSM (master only) #1381
Conversation
On Canon bodies and lens firmware 1.x, this lens identifies itself using model number 150. Starting with firmware 2.x, this lens now identifies itself using model number 368.
|
Thanks for submitting that @lbschenkel. I kept my word and gave you a mention in the forward of the book! I've asked @piponazo to review this. I never submit changes to 'master', so I'm sure @piponazo will do that when the CI is green. |
| @@ -872,7 +872,7 @@ namespace Exiv2 { | |||
| { 137, "Tamron SP 60mm f/2 Macro Di II" }, // 12 | |||
| { 137, "Sigma 10-20mm f/3.5 EX DC HSM" }, // 13 | |||
| { 137, "Tamron SP 24-70mm f/2.8 Di VC USD" }, // 14 | |||
| { 137, "Sigma 18-35mm f/1.8 DC HSM" }, // 15 | |||
| { 137, "Sigma 18-35mm f/1.8 DC HSM | A" }, // 15 | |||
There was a problem hiding this comment.
I am a little surprised to see that we do not need here the line:
{ 368, printCsLensByFocalLength },
which you included at 0.27-maintenance. Although I do not know this part of the code well enough to determine if it is needed or not.
There was a problem hiding this comment.
It's because in this branch, there's only one entry for lens model 368, so it does not need special disambiguation. The extra commit I included (which you're commenting on) was just to make all 3 entries for the same lens to have identical names. But this is for model 137, which my particular lens never used, and I can't produce the necessary sample files and that's why this is not included in the test suite.
Note that there is a line for 137:
{ 137, printCsLensByFocalLength }, // not tested
There was a problem hiding this comment.
Hang on, gentlemen (@lbschenkel). If this works, let's include it in the code as it may resolve a future "disambiguation". For that matter, is there stuff in 0.27-maintenance that should be ported forward to 'master'?
I "sort of" know this code. However, it's messy. One day we'll fix this "properly" with M2Lscript.
There was a problem hiding this comment.
Sorry, I'm confused now.
If this works
What do you mean by this? Let me clarify things:
-
My PR to branch 0.27 was to fix lens model
368, which is the new lens model reported by this lens on firmware 2.x. On firmware 1.x the lens model was150. That branch correctly recognized model150but not368, so I added it and I also fixed the disambiguation for lens368(added theprintCsLensByFocalLength), which was broken. Then I introduced the tests for models150and368, since I had actual images with those model numbers.
In addition, I noticed that model137also refers to the same lens, but the name was slightly different than the other entries, so I changed it to match and have consistent naming everywhere. This is not part of the fix, and that is why I did it on a separate commit, but for this I didn't add any unit tests since I don't have a single image for this lens with model137(maybe this model was only used in very old firmware?) -- I know I can create one by playing with the model number, but I presumed that you want real metadata from the wild in the test suite, not stuff that was fudged to match the code (the code is supposed to be the one matching what is seen in the wild, not the other way around). -
This PR only introduces the tests, because both lens models
150and368work in this branch. In particular, the single368entry is this particular lens [1], so there's no need to have special disambiguation logic (printCsLensByFocalLength).
In addition, the same extra commit was included here for the same reasons.
[1] I think this is probably another lens detection bug in master, since from the history of 0.27 branch I can see that the additional 368 lenses were ported from exiftool, so those are most likely genuine. The bug in 0.27 was that this "import" overwrote the pre-existing 368 lens.
I hope this clarifies it.
There was a problem hiding this comment.
I think so, @lbschenkel. Thanks for the clarification.
See #1368 and #1373.
Masterdoes not have the bug, however it can benefit from the unit tests.P.S.: Not sure if the name of the PR is appropriate, as it is not really fixing anything, but I'm using the suggested name nonetheless.