Skip to content

Fix_1368 Sigma 18-35mm f/1.8 DC HSM (master only) #1381

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 28, 2020
Merged

Fix_1368 Sigma 18-35mm f/1.8 DC HSM (master only) #1381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f08ee8
Test Sigma 18-35mm f/1.8 DC HSM (firmware 2.x)
lbschenkel Oct 19, 2020
4f931c1
Make lens name consistent with models 150 and 368
lbschenkel Oct 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/canonmn_int.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@lbschenkel lbschenkel Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@lbschenkel lbschenkel Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm confused now.

If this works

What do you mean by this? Let me clarify things:

  1. 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 was 150. That branch correctly recognized model 150 but not 368, so I added it and I also fixed the disambiguation for lens 368 (added the printCsLensByFocalLength), which was broken. Then I introduced the tests for models 150 and 368, since I had actual images with those model numbers.
    In addition, I noticed that model 137 also 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 model 137 (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).

  2. This PR only introduces the tests, because both lens models 150 and 368 work in this branch. In particular, the single 368 entry 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, @lbschenkel. Thanks for the clarification.

{ 137, "Sigma 12-24mm f/4.5-5.6 DG HSM II" }, // 16
{ 138, "Canon EF 28-80mm f/2.8-4L" },
{ 139, "Canon EF 400mm f/2.8L" },
Expand Down
Binary file added test/data/Canon_Sigma_18_35_F18_DC_HSM_firmware_1xx.exv
View file
Open in desktop
Binary file not shown.
Binary file added test/data/Canon_Sigma_18_35_F18_DC_HSM_firmware_2xx.exv
View file
Open in desktop
Binary file not shown.
19 changes: 19 additions & 0 deletions tests/bugfixes/github/test_issue_1368.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# -*- coding: utf-8 -*-

import system_tests

class Canon_Sigma_18_35_F18_DC_HSM(metaclass=system_tests.CaseMeta):
url = "https://github.com/Exiv2/exiv2/issues/1368"

filename1 = "$data_path/Canon_Sigma_18_35_F18_DC_HSM_firmware_1xx.exv"
filename2 = "$data_path/Canon_Sigma_18_35_F18_DC_HSM_firmware_2xx.exv"
commands = ["$exiv2 -pa -K Exif.CanonCs.LensType $filename1",
"$exiv2 -pa -K Exif.CanonCs.LensType $filename2"]
stderr = ["", ""]
stdout = [
"""Exif.CanonCs.LensType Short 1 Sigma 18-35mm f/1.8 DC HSM | A
""",
"""Exif.CanonCs.LensType Short 1 Sigma 18-35mm f/1.8 DC HSM | A
"""
]
retval = [0, 0]