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.

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

Fix Canon lens detection (#2057) #2235

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix Canon lens detection (#2057) #2235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8d5a3a
Fix Canon lens detection (#2057)
postscript-dev May 11, 2022
315ae6d
Revert "Fix Canon lens detection (#2057)"
postscript-dev May 27, 2022
8d36bcc
Update Canon decoder for lens detection (#2057)
postscript-dev May 27, 2022
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
15 changes: 11 additions & 4 deletions src/canonmn_int.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2527,6 +2527,13 @@ std::ostream& printCsLensTypeByMetadata(std::ostream& os, const Value& value, co
return printCsLensFFFF(os, value, metadata);
}

// TODO: The lens identification could be improved further:
// 1. RF lenses also set Exif.CanonFi.RFLensType. If a lens cannot be found here then
// the RF mechanism could be used instead.
// 2. Exif.Photo.LensModel and Exif.Canon.LensModel provide a text description of the lens
// (e.g., "RF24-105mm F4-7.1 IS STM" - Note: no manufacturer or space after "RF").
// After parsing, the values could be used to search in the lens array.

// get the values we need from the metadata container
ExifKey lensKey("Exif.CanonCs.Lens");
auto pos = metadata->findKey(lensKey);
Expand All @@ -2540,14 +2547,14 @@ std::ostream& printCsLensTypeByMetadata(std::ostream& os, const Value& value, co
auto const exifFlMin = static_cast<int>(static_cast<float>(pos->value().toInt64(1)) / pos->value().toFloat(2));
auto const exifFlMax = static_cast<int>(static_cast<float>(pos->value().toInt64(0)) / pos->value().toFloat(2));

ExifKey aperKey("Exif.CanonCs.MaxAperture");
ExifKey aperKey("Exif.CanonCs.MinAperture");
pos = metadata->findKey(aperKey);
if (pos == metadata->end() || pos->value().count() != 1 || pos->value().typeId() != unsignedShort) {
os << "Unknown Lens (" << lensType << ")";
return os;
}

auto exifAperMax = fnumber(canonEv(static_cast<int16_t>(pos->value().toInt64(0))));
auto exifAperMin = fnumber(canonEv(static_cast<int16_t>(pos->value().toInt64(0))));

// regex to extract short and tele focal length, max aperture at short and tele position
// and the teleconverter factor from the lens label
Expand Down Expand Up @@ -2588,8 +2595,8 @@ std::ostream& printCsLensTypeByMetadata(std::ostream& os, const Value& value, co
auto aperMaxTele = std::stof(base_match[4].str()) * tc;
auto aperMaxShort = base_match[3].length() > 0 ? std::stof(base_match[3].str()) * tc : aperMaxTele;

if (flMin != exifFlMin || flMax != exifFlMax || exifAperMax < (aperMaxShort - .1 * tc) ||
exifAperMax > (aperMaxTele + .1 * tc)) {
if (flMin != exifFlMin || flMax != exifFlMax || exifAperMin < (aperMaxShort - .1 * tc) ||
exifAperMin < (aperMaxTele + .1 * tc)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that I understand this change to use exifAperMin 🤔

From the lens description we get min & max focal length (24mm, 105mm) and we get maximum aperture at the short and tele end. (F4, F7.1)

Now if a lens is a possible match the maximum reported aperture should be between the aperMaxShort and aperMaxTele.

I'm not sure how exifAperMin which in this case is F40, would help in filtering out lenses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hassec:

I'm not sure that I understand this change to use exifAperMin 🤔

Before this PR, the values in the user's test file are:

$ exiv2 --Print kyvt --key Exif.CanonCs.LensType --key Exif.CanonCs.MaxAperture --key Exif.CanonCs.MinAperture --key Exif.Photo.FNumber Canon_RF_24-105_F4-7.1_IS_STM.jpg
Exif.Photo.FNumber                           Rational   71/10  F7.1
Exif.CanonCs.LensType                        Short      61182  Unknown Lens (61182)
Exif.CanonCs.MaxAperture                     Short      184  F7.3
Exif.CanonCs.MinAperture                     Short      340  F40

The Exif.CanonCs.MaxAperture tag has a value of F7.3 but we are expecting this to be F7.1 to fit in with the lens specification (Canon RF 24-105mm F4-7.1 IS STM). As the value is outside the range that we are expecting, a quick fix was to change over to using Exif.CanonCs.MinAperture instead. I realised that this would not filter much but seemed the best option available at the time.

Looking again at the problem, the test file has Exif.Photo.FNumber at F7.1, so the photo looks as though it was taken at the maximum aperture for the lens. From other examples, Exif.CanonCs.MaxAperture looks as though it is usually set to the maximum value of the lens (maybe Exif.CanonCs.MinAperture is the value that the camera supports?).

Perhaps the user's problem lies instead with our understanding of how Canon stores values. We calculate the exifAperMax variable by decoding the metadata value using:

exiv2/src/canonmn_int.cpp

Lines 2814 to 2834 in 1ff0950

float canonEv(int64_t val) {
// temporarily remove sign
int sign = 1;
if (val < 0) {
sign = -1;
val = -val;
}
// remove fraction
const auto remainder = val & 0x1f;
val -= remainder;
auto frac = static_cast<float>(remainder);
// convert 1/3 (0x0c) and 2/3 (0x14) codes
if (frac == 0x0c) {
frac = 32.0F / 3;
} else if (frac == 0x14) {
frac = 64.0F / 3;
} else if ((val == 160) && (frac == 0x08)) { // for Sigma f/6.3 lenses that report f/6.2 to camera
frac = 30.0F / 3;
}
return sign * (val + frac) / 32.0F;
}

I have tried experimenting by reverting my commit and added an exception to the canonEv() function. The user's lens was correctly identified and a knock-on effect in another Canon test file, changed Exif.CanonSi.ShutterSpeedValue to be more consistent with Exif.Photo.ShutterSpeedValue.

I think that this is a better solution. If you are happy with this approach, then I will make the changes.

Copy link
Member

Choose a reason for hiding this comment

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

The problem of the Aperture value is that it's encoding and decoding aren't perfect, which is why we get these slightly incorrect values like 7.3 instead of 7.1.

Can you double-check how exiftool handles this specific canon transformation?

In general I'm ok with improving canonEV but we'd have to somehow make sure it doesn't have a negative impact on other exiv2 outputs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hassec:

Can you double-check how exiftool handles this specific canon transformation?

I forgot to mention that with ExifTool, the user's test file also outputs Exif.CanonCs.MaxAperture as 7.3. On the code in git, apart from our Sigma exception, the canonEV() function looks the same on ExifTool and Exiv2.

In general I'm ok with improving canonEV but we'd have to somehow make sure it doesn't have a negative impact on other exiv2 outputs.

Agreed. When I ran the Exiv2 testing on my changes, only 1 problem was flagged up.

======================================================================
ERROR: Sigma_50mm_F1.4_DG_HSM_A_for_EOS.exv_test (test_regression_allfiles.TestAllFiles)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:/Users/postscript-dev/source_code/exiv2_postscript-dev/tests/regression_tests/test_regression_allfiles.py", line 169, in test_func
    BT.reportTest(os.path.basename(filename), out)
  File "C:/Users\postscript-dev\source_code\exiv2_postscript-dev\tests/bash_tests/utils.py", line 567, in reportTest
    raise RuntimeError('\n' + log.to_str())
RuntimeError: 
[ERROR] The output of the testcase mismatch the reference
[INFO] The output has been saved to file C:/Users/postscript-dev/source_code/exiv2_postscript-dev/test/tmp/Sigma_50mm_F1.4_DG_HSM_A_for_EOS.exv.out
[INFO] simply_diff:
C:/Users/postscript-dev/source_code/exiv2_postscript-dev/test/data/test_reference_files/Sigma_50mm_F1.4_DG_HSM_A_for_EOS.exv.out: 227 lines
C:/Users/postscript-dev/source_code/exiv2_postscript-dev/test/tmp/Sigma_50mm_F1.4_DG_HSM_A_for_EOS.exv.out: 227 lines
The first mismatch is in line 90:
< Exif.CanonSi.ShutterSpeedValue               Short       1  184  1/54 s
> Exif.CanonSi.ShutterSpeedValue               Short       1  184  1/50 s

Looking at the test/data/test_reference_files/Sigma_50mm_F1.4_DG_HSM_A_for_EOS.exv.out file, I can see that it has

Exif.Photo.ShutterSpeedValue                 SRational   1  368640/65536  1/49 s

Although we don't have the makernotes specifications, the new Exif.CanonSi.ShutterSpeedValue value of 1/50 is closer to the Exif.Photo.ShutterSpeedValue value of 1/49 which looks like an improvement.

Do you have something else that you would like me to test for?

continue;
}

Expand Down
Binary file added test/data/issue_2057_poc1.exv
View file
Open in desktop
Binary file not shown.
490 changes: 490 additions & 0 deletions test/data/test_reference_files/issue_2057_poc1.exv.out
View file
Open in desktop

Large diffs are not rendered by default.

17 changes: 17 additions & 0 deletions tests/bugfixes/github/test_issue_2057.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# -*- coding: utf-8 -*-

from system_tests import CaseMeta, path

class PanasonicMakerPrintAccelerometerIntOverflow(metaclass=CaseMeta):
"""
Regression test for the bug described in:
https://github.com/Exiv2/exiv2/issues/2057
"""
url = "https://github.com/Exiv2/exiv2/issues/2057"

filename = path("$data_path/issue_2057_poc1.exv")
commands = ["$exiv2 --Print kyyvt --key Exif.CanonCs.LensType $filename"]
stderr = [""]
stdout = ["""Exif.CanonCs.LensType Short 61182 Canon RF 24-105mm F4L IS USM *OR* Canon RF 24-105mm F4-7.1 IS STM
"""]
retval = [0]
12 changes: 5 additions & 7 deletions tests/lens_tests/test_canon_lenses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@
from lens_tests.utils import extract_lenses_from_cpp, make_test_cases, aperture_to_raw_exif

# NOTE
# Normally the canon maker note holds the max aperture of the lens at the focal length
# the picture was taken at. Thus for a f/4-6.3 lens, this value could be anywhere in that range.
# For the below tests we only test the scenario where the lens was used at it's shortest focal length.
# Thus we always pick the 'aperture_max_short' of a lens as the value to write into the
# Exif.CanonCs.MaxAperture field.
# Normally the canon makernote holds the minimum aperture used by the camera (in
# Exif.CanonCs.MinAperture). For the tests below, we set this tag to the minimum
# aperture of the lens.

# get directory of the current file
file_dir = os.path.dirname(os.path.realpath(__file__))
Expand All @@ -32,14 +30,14 @@
{
"filename": "$data_path/template.exv",
"commands": [
'$exiv2 -M"set Exif.CanonCs.LensType $lens_id" -M"set Exif.CanonCs.Lens $focal_length_max $focal_length_min 1" -M"set Exif.CanonCs.MaxAperture $aperture_max" $filename && $exiv2 -pa -K Exif.CanonCs.LensType $filename'
'$exiv2 -M"set Exif.CanonCs.LensType $lens_id" -M"set Exif.CanonCs.Lens $focal_length_max $focal_length_min 1" -M"set Exif.CanonCs.MinAperture $aperture_min" $filename && $exiv2 -pa -K Exif.CanonCs.LensType $filename'
],
"stderr": [""],
"stdout": ["Exif.CanonCs.LensType Short 1 $lens_description\n"],
"retval": [0],
"lens_id": lens_tc["id"],
"lens_description": lens_tc["target"],
"aperture_max": aperture_to_raw_exif(lens_tc["aperture_max_short"] * lens_tc["tc"]),
"aperture_min": aperture_to_raw_exif(lens_tc["aperture_min_short"] * lens_tc["tc"]),
"focal_length_min": int(lens_tc["focal_length_min"] * lens_tc["tc"]),
"focal_length_max": int(lens_tc["focal_length_max"] * lens_tc["tc"]),
},
Expand Down
13 changes: 6 additions & 7 deletions tests/lens_tests/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,9 @@ def lens_is_match(l1, l2):
"""
Test if lens l2 is compatible with lens l1

This assumes we write l1's metadata and pick its 'aperture_max_short' value
as the maximum aperture value to write into exif.
Normally the canon maker note holds the max aperture of the lens at the focal length
the picture was taken at. Thus for a f/4-6.3 lens, this value could be anywhere in that range.
This assumes we write l1's metadata and pick its 'aperture_min_short' value
as the minimum aperture value to write into exif.
Normally the canon makernote holds the minimum aperture of the camera.
"""
# the problem is that the round trip transformation isn't exact
# so we need to account for this here as well to not define a target
Expand All @@ -132,9 +131,9 @@ def lens_is_match(l1, l2):
[
l1["focal_length_min"] * l1["tc"] == l2["focal_length_min"] * l2["tc"],
l1["focal_length_max"] * l1["tc"] == l2["focal_length_max"] * l2["tc"],
(l2["aperture_max_short"] - 0.1) * l2["tc"]
<= reconstructed_aperture
<= (l2["aperture_max_tele"] + 0.1) * l2["tc"],
(l2["aperture_min_short"] - 0.1) * l2["tc"]
>= reconstructed_aperture
<= (l2["aperture_min_tele"] + 0.1) * l2["tc"],
]
)

Expand Down