Update canon tags 2

[clanmills]

This super-cedes #1628. Test 981 is failing because the output from Exif.CanonAF2Id tags have the wrong type/value.

[codecov]

Codecov Report

Merging #2087 (e1651ba) into main (e3ca59b) will increase coverage by 0.55%.
The diff coverage is 62.74%.

❗ Current head e1651ba differs from pull request most recent head 2f135f7. Consider uploading reports for the commit 2f135f7 to get more accurate results

@@            Coverage Diff             @@
##             main    #2087      +/-   ##
==========================================
+ Coverage   63.19%   63.75%   +0.55%     
==========================================
  Files          96       96              
  Lines       19204    19202       -2     
  Branches     9797     9798       +1     
==========================================
+ Hits        12136    12242     +106     
+ Misses       4762     4658     -104     
+ Partials     2306     2302       -4     

Impacted Files	Coverage Δ
include/exiv2/basicio.hpp	100.00% <ø> (ø)
include/exiv2/bmffimage.hpp	100.00% <ø> (ø)
include/exiv2/error.hpp	65.51% <ø> (ø)
include/exiv2/exif.hpp	100.00% <ø> (ø)
include/exiv2/pgfimage.hpp	100.00% <ø> (ø)
src/canonmn_int.cpp	75.14% <ø> (+6.14%)	⬆️
src/error.cpp	74.41% <ø> (ø)
src/exif.cpp	66.01% <ø> (ø)
src/futils.cpp	72.06% <ø> (ø)
src/preview.cpp	50.00% <ø> (ø)
... and 28 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 e3ca59b...2f135f7. Read the comment docs.

[piponazo]

👀 how is possible that this was working previously?

[clanmills]

Yes. The Canon.AFxxx behaviour is unchanged since 0.27.3. I modified the test slightly because a couple of new fields were add Exif.Canon.AFMicroAdj and Exif.Canon.AFConfig. I don't know the genesis of these fields. @piponazo @alexvanderberkel @hassec and @neheb get a mention in the blame listing:

AFConfig

534 rmills@rmillsm1:~/gnu/github/exiv2/main $ git blame src/canonmn_int.cpp | grep AFConfig 
bdd8a386b src/canonmn_int.cpp (Christoph Hasse    2021-06-08 21:56:04 +0200  637)         {0x4028, "AFConfig", N_("AFConfig"), N_("AFConfig"), canonId, makerTags, unsignedShort, -1, printValue},
ff2ffb190 src/canonmn_int.cpp (Alex Esseling      2021-01-12 20:55:29 +0100 1230)     //Canon AFConfig Tags
6da49fd29 src/canonmn_int.cpp (Rosen Penev        2021-05-17 16:54:53 -0700 1232)          {0x0001, "AFConfigTool", N_("AF Config Tool"), N_("AF Config Tool"), canonAfCId, makerTags, signedLong, -1, printValue},

AFMicroAdj

535 rmills@rmillsm1:~/gnu/github/exiv2/main $ git blame src/canonmn_int.cpp | grep AFMicroAdj
bdd8a386b src/canonmn_int.cpp (Christoph Hasse    2021-06-08 21:56:04 +0200  628)         {0x4013, "AFMicroAdj", N_("AFMicroAdj"), N_("AFMicroAdj"), canonId, makerTags, unsignedShort, -1, printValue},
ff2ffb190 src/canonmn_int.cpp (Alex Esseling      2021-01-12 20:55:29 +0100  906)     // Canon AFMicroAdjMode Quality Info, tag 0x0001
6da49fd29 src/canonmn_int.cpp (Rosen Penev        2021-05-17 16:54:53 -0700  907)     constexpr TagDetails canonAFMicroAdjMode[] = {
bdd8a386b src/canonmn_int.cpp (Christoph Hasse    2021-06-08 21:56:04 +0200  914)      // Canon AFMicroAdj Info Tag
6da49fd29 src/canonmn_int.cpp (Rosen Penev        2021-05-17 16:54:53 -0700  916)          {0x0001, "AFMicroAdjMode", N_("AFMicroAdjMode"), N_("AFMicroAdjMode"), canonAfMiAdjId, makerTags, signedLong, -1, EXV_PRINT_TAG(canonAFMicroAdjMode)},
bdd8a386b src/canonmn_int.cpp (Christoph Hasse    2021-06-08 21:56:04 +0200  917)          {0x0002, "AFMicroAdjValue", N_("AF Micro Adj Value"), N_("AF Micro Adj Value"), canonAfMiAdjId, makerTags, signedRational, -1, printValue},
de036b2b2 src/canonmn_int.cpp (Luis Díaz Más      2022-02-09 23:00:03 +0100  918)          {0xffff, "(UnknownCanonAFMicroAdjTag)", "(UnknownCanonAFMicroAdjTag)", N_("Unknown Canon AFMicroAdj tag"), canonAfMiAdjId, makerTags, signedShort, 1, printValue}
536 rmills@rmillsm1:~/gnu/github/exiv2/main $ 

[piponazo]

Great to see the tests finally passing! Good team work 💪 @alexvanderberkel @clanmills .

Is the work in this branch finished? Or do you have plans to keep working here?

[alexvanderberkel]

Thanks @clanmills for helping me out on the last meters to let the CI pass! As always I enjoy working with you. @piponazo I would rather stop working on this PR and do cosmetic work in different PRs.

[hassec]

Just a few small points right now.

[clanmills]

The code correctly reads LensSerialNumber on the attached ExifTool test file.

935 rmills@rmillsmm-local:~/temp $ ls -l CanonEOS_R6.jpg 
-rw-r--r--+ 1 rmills  staff  55545 11 Feb 13:23 CanonEOS_R6.jpg
936 rmills@rmillsmm-local:~/temp $ exiftool CanonEOS_R6.jpg  | grep Lens | grep Serial
Lens Serial Number              : 8003003431
937 rmills@rmillsmm-local:~/temp $ exiv2 -K Exif.Photo.LensSerialNumber CanonEOS_R6.jpg 
Exif.Photo.LensSerialNumber                  Ascii      11  8003003431
938 rmills@rmillsmm-local:~/temp $ 

The code also successfully modifies LensSerialNumber

938 rmills@rmillsmm-local:~/temp $ cp CanonEOS_R6.jpg  foo.jpg
939 rmills@rmillsmm-local:~/temp $ exiv2 -M'set Exif.Photo.LensSerialNumber 1343003008' foo.jpg 
940 rmills@rmillsmm-local:~/temp $ exiv2 -K Exif.Photo.LensSerialNumber foo.jpg 
Exif.Photo.LensSerialNumber                  Ascii      11  1343003008

The code also successfully creates a .EXV file which can be read:

941 rmills@rmillsmm-local:~/temp $ exiv2 -ee --force foo.jpg
942 rmills@rmillsmm-local:~/temp $ exiv2 -K Exif.Photo.LensSerialNumber foo.exv 
Exif.Photo.LensSerialNumber                  Ascii      11  1343003008
943 rmills@rmillsmm-local:~/temp $ 

[alexvanderberkel]

I think we are actually done with the PR or have I missed anything?

[clanmills]

Gentlemen: I believe this is working and can be merged.

@alexvanderberkel. We should have a bug report to justify those changes. I believe lots of new tags have been introduced and it would be good to have a bug report that says "Please provide support for the following Canon MakerNote Tags" and provide a list.

Before merging and closing this PR, it would be good to see evidence that:

1. The Tags listed in the bug report have been tested (using the Exiv2 Canon Test Images)

• ideally, test should be added the exiv2 test suite for the new tags.
• if this requires adding lots of "fat" images to the test suite, terminal transcript evidence in this PR is sufficient.

2. Compare exiv2 v ExifTool for the tags listed in the bug report.

@1div0. Can you help @alex with this? Obtaining the ExifTool test images is documented in my book: https://exiv2.org/book/index.html#8-6

[piponazo]

Gentlemen: I believe this is working and can be merged.

However many of the CI jobs are failing. Until all the relevant CI jobs are passing we should not merge this PR.

[clanmills]

Crap. I don't see that. The last time I looked, they were green.

@alexvanderberkel is kind of busy. This PR has been hanging about for 6 months. How are we going to get this done?

[piponazo]

I can try to take a quick look tonight and check if is something easy to fix (hopefully just some wrong expectations in the tests). I would also love to see this merged soon. I do not like to have nice contributions blocked due to small details.

[clanmills]

I'll investigate this CI failures. @alexvanderberkel Can you please submit a bug report enumerating the keys to be supported. When the CI is green, perhaps @1div0 and/or @sridharb1 can contribute to getting this tested.

[hassec]

it's just the new tests that are failing because they were created without the new output of this MR.

But the test automatically creates new reference files with the changes and stores them in a tmp folder. This is reported in the ouput:

Error:  The output of the testcase mismatch the reference
[INFO] The output has been saved to file D:\a\exiv2\exiv2\test\tmp\CanonEF100mmF2.8LMacroISUSM.exv.out
[INFO] simply_diff:
....

So I'd suggest that someone should just run the test locally, copy the new references from exiv2/test/tmp over into exiv2/test/test_reference_files/ and commit and push them.

Then we can all have a look together if the diff makes sense and then hopefully merge 🎉

[clanmills]

Thanks @hassec for the explanation. And Thanks @postscript-dev for your web/documentation changes.

Q. How did regressionTest get into this PR? It hadn't been invented when we were working on this PR last week.
Q. Has regressionTest been documented in README.md when test types bash, bugfix, unit, lens, tiff and version are discussed?

It is regression that's failing on macOS (and presumably everywhere else). I'll fix and update this PR.

[clanmills]

It must be Christmas. All the lights have turned Green!.

[postscript-dev]

@clanmills: For some reason I couldn't reply to the review above, so have done so here.

@hassec I believe this is included in the PR. I've linked #2059 to this PR so it closed when this is merged.

581 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ grep -n 'Canon RF 50mm F1.8 STM' ../src/*.cpp 
../src/canonmn_int.cpp:2019:        {61182, "Canon RF 50mm F1.8 STM"                                    }, // 8
../src/canonmn_int.cpp:2477:        { 280, N_("Canon RF 50mm F1.8 STM")  }

Do you know why it's in two places, @postscript-dev ? It's localised in one N_("....") and not in the other.

I have only started using Canon files recently but I see that Canon adds more than one lens related tag. For example, in this file from #2057, there are:

$ exiv2 -g lens/i Canon_RF_24-105_F4-7.1_IS_STM.jpg
Exif.CanonCs.LensType                        Short       1  Unknown Lens (61182)
Exif.CanonCs.Lens                            Short       3  24.0 - 105.0 mm
Exif.CanonFi.RFLensType                      SShort      1  Canon RF 24-105mm F4-7.1 IS STM
Exif.CanonCf.LensAFStopButton                Short       1  0
Exif.Canon.LensModel                         Ascii     138  RF24-105mm F4-7.1 IS STM
Exif.CanonLe.LensSerialNumber                SLong       1  -1509818368
Exif.CanonAfC.USMLensElectronicMF            SLong       1  Enable After AF
Exif.CanonAfC.LensDriveWhenAFImpossible      SLong       1  Continue Focus Search
Exif.Photo.LensSpecification                 Rational    4  24/1 105/1 0/1 0/1
Exif.Photo.LensModel                         Ascii      25  RF24-105mm F4-7.1 IS STM
Exif.Photo.LensSerialNumber                  Ascii      11  000002a620

(note: In #2057, I am going to fix the Exif.CanonCs.LensType translated output).

The two values that you found in src/canonmn_int.cpp are from the Exif.CanonCs.LensType and Exif.CanonFi.RFLensType respectively. It is possible that these tags may not always be the same value.

I looked at some of the lens lists in other makernotes and I didn't see the N_("...") function used. As any string we use N_("...") with appears in the Exiv2 localisation project, it might be better to remove them in this case.

[clanmills]

Ah right, thanks @postscript-dev. Lens recognition is horrible.

You're right. That string should probably not be localised. Strings pass unchanged if not translated.

I'm happy with your explanation that CanonCs and CanonFi are different families (and probably mutually exclusive).

If I have sufficient permission, I'll mark this "change request blocker" as resolved.

I hope than @alexvanderberkel will provide an issue report to enumerate the tags which are intended to be modified/recognised in this PR.

If there are no further "blockers" and the CI is green I hope somebody will approve and merge this PR.

[clanmills]

We still have "1 change requested". I can't find the outstanding request.