Interpretation for Exif.Photo.LensSpecification #2422

norbertwg · 2022-11-15T15:55:46Z

new method printLensSpecification is copied from old Nikon3MakerNote::print0x0084
call of Nikon3MakerNote::print0x0084 replaced by call of printLensSpecification

Exif.Photo.LensSpecification and Exif.Nikon3.Lens added to EasyAccess API

printLensSpecification is copied from old Nikon3MakerNote::print0x0084 call of Nikon3MakerNote::print0x0084 replaced by printLensSpecification

Exif.Photo.LensSpecification and Exif.Nikon3.Lens

test/data/test_reference_files/IMG_1361.dng.out

test/data/test_reference_files/IMG_3578.heic.out

test/data/test_reference_files/Sigma_14-24mm_F2.8_DG_HSM_A_for_EOS.exv.out

test/data/test_reference_files/exiv2-bug1145c.exv.out

postscript-dev · 2022-11-16T20:31:38Z

@norbertwg
Thanks for working on this, I can cross it off my list. I have a couple of suggestions.

At the beginning of printLensSpecification(), could you check that the type of the tag is correct (i.e. add value.typeId() != unsignedRational).

Could you please check that both of the lower values in Exif.Photo.LensSpecification are less than the higher values. If this is not the case, then you could output the vanilla value in ( ).

norbertwg · 2022-11-17T19:04:09Z

@kmilos @postscript-dev
Thanks for the valuable feedback. The suggestions are sligthly different, so I would like to align the rules with you before I finalise the changes.

No change for the logic, if all four values are positive: output will look like "100mm F2.8", "70-300mm F4-5.6", "16-50mm F2.8".
In the sample pictures I found several entries, where the dividend of all four values is zero (e.g. 0/10 0/10 0/10 0/10). This I regard as indication, that no lens information is stored and suggest to print it as "n/a". I Suggest "n/a" as this is done already for several tags as well, whereas I did not find an example with "unknown" or "unspecified".
Some images had a dividend of zero only for the F-numbers. In this case I suggest to just print the focal length information (e.g. "35-80mm",
In all other cases I regard the values as wrong and suggest to print vanilla value in ( ). This includes:
-- at least one divisor is zero
-- wrong type
-- wrong number of values
-- one value for focal length is zero
-- one value for F-number is zero
For the last two the alternative could be to print the zero-value as "n/a" and the other value as number (e.g. "n/a-80mm"). But this looks ugly and as I found no example for them, I believe, that this can be also considered as a wrong entry.

kmilos · 2022-11-18T08:16:19Z

There is also the clear case in the Exif spec:

When the minimum F number is unknown, the notation is 0/0.

postscript-dev · 2022-11-18T09:27:14Z

@kmilos
Ahh, I was looking for the case of all zeros, not just the focal length. I am happy to go with your explanation.

norbertwg · 2022-11-19T10:21:34Z

Rework according review is done:
Focal lenght is printed with decimal digits (if needed), e.g. the funny 807365/524263 is printed as 1.54mm.
Values where dividend is zero (0/10 and also 0/0) are regarded as unknown and will printed as "n/a".
The rule what is regareded as wrong is changed:
-- wrong type
-- wrong number of values
-- first value is bigger than second value
-- one devisor is zero where associated dividend is not zero (e.g. 7/0)

A test file is created to cover the special cases, which do not occur in regression tests. For completeness the file includes also test for the standard scenarios, which also occur in regression tests.

codecov · 2022-11-19T10:37:55Z

Codecov Report

Merging #2422 (979ce8a) into main (3dd881f) will decrease coverage by 0.06%.
The diff coverage is 98.03%.

❗ Current head 979ce8a differs from pull request most recent head 10bbe3f. Consider uploading reports for the commit 10bbe3f to get more accurate results

@@            Coverage Diff             @@
##             main    #2422      +/-   ##
==========================================
- Coverage   64.64%   64.59%   -0.06%     
==========================================
  Files         119      103      -16     
  Lines       21119    22224    +1105     
  Branches    10420    10857     +437     
==========================================
+ Hits        13653    14356     +703     
- Misses       5300     5633     +333     
- Partials     2166     2235      +69

Impacted Files	Coverage Δ
src/nikonmn_int.cpp	`61.22% <ø> (-0.68%)`	⬇️
src/tags_int.hpp	`91.34% <ø> (+0.16%)`	⬆️
src/tags_int.cpp	`80.11% <98.00%> (+2.36%)`	⬆️
src/easyaccess.cpp	`94.44% <100.00%> (+0.13%)`	⬆️
src/cr2header_int.hpp	`0.00% <0.00%> (-33.34%)`	⬇️
include/exiv2/tags.hpp	`0.00% <0.00%> (-33.34%)`	⬇️
src/tiffcomposite_int.hpp	`90.09% <0.00%> (-2.64%)`	⬇️
src/tiffimage_int.hpp	`77.77% <0.00%> (-2.23%)`	⬇️
src/tags.cpp	`65.74% <0.00%> (-2.10%)`	⬇️
src/gifimage.cpp	`34.14% <0.00%> (-0.86%)`	⬇️
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

norbertwg · 2022-11-26T10:32:09Z

Three checks are failing, in Run Conan and in Set up Cygwin, which I believe is not a result of this PR.
@postscript-dev @kmilos
Could you please finish your review, so that I can regard my changes as approved (or make the needed adjustments)?
After that somebody should decide, how to deal with the failing checks.

postscript-dev · 2022-11-26T23:07:29Z

@norbertwg

Could you please finish your review,

Unless someone else wants to, I will look at this tomorrow.

…ification

norbertwg · 2023-02-18T15:34:43Z

This pull request waits for approving review. According to my understanding, all review comments so far are covered - or is something still missing from my side?

postscript-dev · 2023-02-20T21:49:12Z

@norbertwg

This pull request waits for approving review. According to my understanding, all review comments so far are covered - or is something still missing from my side?

Sorry for not looking at this sooner, recently I haven't worked much on open source projects. Unless someone else wants to, I will try and find some time to review this in the next couple of days.

neheb · 2023-03-05T22:39:30Z

needs rebase

printLensSpecification is copied from old Nikon3MakerNote::print0x0084 call of Nikon3MakerNote::print0x0084 replaced by printLensSpecification add two lens tags to EasyAccess API Exif.Photo.LensSpecification and Exif.Nikon3.Lens handling of zero; more precision for focal length silence compiler warnings

norbertwg · 2023-03-06T10:05:07Z

@neheb
Rebase is done; as it was my first, I hope I have done it correct.

neheb · 2023-03-07T01:59:27Z

need to push it.

git push --force

postscript-dev · 2023-03-07T08:59:44Z

@norbertwg
You can find more information on rebasing in https://github.com/Exiv2/exiv2/blob/main/GIT_GUIDELINES.md#keeping-the-history-linear

piponazo

The changes look good to me.

@neheb mentioned that the PR needs to be rebased, so I will leave for him the final approval. I have been a long time without contributing actively to the project and I might be missing something.

piponazo · 2023-03-07T09:37:07Z

src/tags_int.cpp

-    os << "-" << len2;
+  // values numerically are ok, so they can be converted
+  // here first and second can be zero, so initialise float with 0.0
+  float focalLength1 = 0.0;


[suggestion] Some compilers at certain warning levels would complain about not using the same types here (float variable vs double literals). Could you replace those 0.0 with 0.0F when comparing or assigning float variables?

valid point, I changed it.

…ps://github.com/norbertwg/exiv2 into interpretation-for-Exif.Photo.LensSpecification

ghost · 2023-03-07T10:45:50Z

👇 Click on the image for a new way to code review

Legend

norbertwg · 2023-03-07T14:39:10Z

@postscript-dev : thanks for the useful link
@neheb : push is done

norbertwg added 2 commits November 14, 2022 17:32

interpretation for ExifPhoto.LensSpecification

eb47fcf

printLensSpecification is copied from old Nikon3MakerNote::print0x0084 call of Nikon3MakerNote::print0x0084 replaced by printLensSpecification

add two lens tags to EasyAccess API

5116598

Exif.Photo.LensSpecification and Exif.Nikon3.Lens

kmilos reviewed Nov 16, 2022

View reviewed changes

handling of zero; more precision for focal length

6066130

silence compiler warnings

5d0d08f

postscript-dev assigned norbertwg Nov 27, 2022

postscript-dev added the enhancement feature / functionality enhancements label Nov 27, 2022

Merge branch 'Exiv2:main' into interpretation-for-Exif.Photo.LensSpec…

5e1a1bc

…ification

piponazo reviewed Mar 7, 2023

View reviewed changes

Merge branch 'interpretation-for-Exif.Photo.LensSpecification' of htt…

979ce8a

…ps://github.com/norbertwg/exiv2 into interpretation-for-Exif.Photo.LensSpecification

to avoid compiler warnings: 0.0 changed to 0.0f

10bbe3f

neheb approved these changes Mar 7, 2023

View reviewed changes

neheb merged commit ff7bfb3 into Exiv2:main Mar 7, 2023

nehaljwani mentioned this pull request Apr 30, 2023

Is a new release happening soon? #2406

Closed

norbertwg deleted the interpretation-for-Exif.Photo.LensSpecification branch July 30, 2023 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interpretation for Exif.Photo.LensSpecification #2422

Interpretation for Exif.Photo.LensSpecification #2422

norbertwg commented Nov 15, 2022

postscript-dev commented Nov 16, 2022

norbertwg commented Nov 17, 2022

kmilos commented Nov 18, 2022

postscript-dev commented Nov 18, 2022

norbertwg commented Nov 19, 2022

codecov bot commented Nov 19, 2022 •

edited

Loading

norbertwg commented Nov 26, 2022

postscript-dev commented Nov 26, 2022

norbertwg commented Feb 18, 2023

postscript-dev commented Feb 20, 2023

neheb commented Mar 5, 2023

norbertwg commented Mar 6, 2023

neheb commented Mar 7, 2023

postscript-dev commented Mar 7, 2023

piponazo left a comment

piponazo Mar 7, 2023

norbertwg Mar 7, 2023

ghost commented Mar 7, 2023 •

edited by ghost

Loading

Legend

norbertwg commented Mar 7, 2023

Interpretation for Exif.Photo.LensSpecification #2422

Interpretation for Exif.Photo.LensSpecification #2422

Conversation

norbertwg commented Nov 15, 2022

postscript-dev commented Nov 16, 2022

norbertwg commented Nov 17, 2022

kmilos commented Nov 18, 2022

postscript-dev commented Nov 18, 2022

norbertwg commented Nov 19, 2022

codecov bot commented Nov 19, 2022 • edited Loading

Codecov Report

norbertwg commented Nov 26, 2022

postscript-dev commented Nov 26, 2022

norbertwg commented Feb 18, 2023

postscript-dev commented Feb 20, 2023

neheb commented Mar 5, 2023

norbertwg commented Mar 6, 2023

neheb commented Mar 7, 2023

postscript-dev commented Mar 7, 2023

piponazo left a comment

Choose a reason for hiding this comment

piponazo Mar 7, 2023

Choose a reason for hiding this comment

norbertwg Mar 7, 2023

Choose a reason for hiding this comment

ghost commented Mar 7, 2023 • edited by ghost Loading

Legend

norbertwg commented Mar 7, 2023

codecov bot commented Nov 19, 2022 •

edited

Loading

ghost commented Mar 7, 2023 •

edited by ghost

Loading