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.

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

Conversation

postscript-dev
Copy link
Collaborator

@postscript-dev postscript-dev commented May 11, 2022

The Canon RF 24-105 F4-7.1 IS STM lens is reported as using an
Exif.CanonCs.MaxAperture of F7.3 instead of F7.1. As a result,
the translation of Exif.CanonCs.LensType is missing.

Fix updates the CanonEV() decode function to provide the correct
value.

Closes #2057.

Previously, when translating `Exif.CanonCs.LensType`,
`Exif.CanonCs.MaxAperture` was used as the upper bound to find a
potential match. In Exiv2#2057, the test file produced a value outside of
that range and the lens was not identified.

The fix uses `Exif.CanonCs.MinAperture` instead.
@postscript-dev postscript-dev added bug lens Issue related to lens detection labels May 11, 2022
@postscript-dev postscript-dev added this to the v1.00 milestone May 11, 2022
@postscript-dev postscript-dev self-assigned this May 11, 2022
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #2235 (8d36bcc) into main (19dc566) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2235      +/-   ##
==========================================
+ Coverage   63.42%   63.47%   +0.05%     
==========================================
  Files         117      118       +1     
  Lines       19593    19614      +21     
  Branches     9551     9567      +16     
==========================================
+ Hits        12426    12450      +24     
+ Misses       5098     5096       -2     
+ Partials     2069     2068       -1     
Impacted Files Coverage Δ
src/canonmn_int.cpp 72.02% <100.00%> (+0.46%) ⬆️
app/app_utils.cpp 33.33% <0.00%> (-12.13%) ⬇️
app/exiv2.cpp 62.55% <0.00%> (-0.03%) ⬇️
app/actions.hpp 100.00% <0.00%> (ø)
app/exiv2app.hpp 100.00% <0.00%> (ø)
src/nikonmn_int.cpp 57.10% <0.00%> (ø)
src/crwimage_int.hpp 93.02% <0.00%> (ø)
src/makernote_int.hpp 92.30% <0.00%> (ø)
src/tiffimage_int.cpp 77.90% <0.00%> (ø)
include/exiv2/value.hpp 84.44% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0561392...8d36bcc. Read the comment docs.

Comment on lines 2598 to 2599
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?

This reverts commit d8d5a3a.

Revert to fix the lens detection in a different way.
The `Canon RF 24-105 F4-7.1 IS STM` lens is reported as using an
`Exif.CanonCs.MaxAperture` of `F7.3` instead of `F7.1`. As a result,
the translation of `Exif.CanonCs.LensType` is missing.

Fix updates the `CanonEV()` decode function to provide the correct
value.
@kevinbackhouse
Copy link
Collaborator

What's the current status on this?

@postscript-dev
Copy link
Collaborator Author

@kevinbackhouse

What's the current status on this?

I will try and take a look tomorrow and then update you.

@postscript-dev
Copy link
Collaborator Author

@kevinbackhouse

What's the current status on this?

The reason for this PR is that the lens in a user's file was incorrectly reported. The problem is that our knowledge of the Canon decoder is incomplete and the data in the user's file mapped to the wrong value. I made a commit that added an exception to the decoding function to correctly report the value.

As the decoder is used in multiple places, the outstanding issue is whether the fix I made might causes problems elsewhere. However, we already have another exception in the decoding code for a Sigma lens so this type of change has already been used for some time without reported issue. I looked in the ExifTool source code and they have the same basic formula but don't have any exceptions.

@hassec
As you have experience of Canon files, do you have any suggestions to move this PR forward?

@kevinbackhouse kevinbackhouse modified the milestones: v0.28.0, Backlog Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug lens Issue related to lens detection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatch lens detection with Canon RF 24-105 versions (STM / USM) in darktable
3 participants