Several Exif keys added in easyaccess

[norbertwg]

Related to issue #2338
For lensmodel the call of findMetadatum is replaced by code based on findMetadatum.
Reason: In all samples I analysed, an image containing Exif.NikonLd4.LensIDNumber also contains Exif.NikonLd4.LensID and vice versa. One of them (sometimes both) has the integer value zero, whereas the other can have a reasonable value. So when looping through the tags, a NikonLd4-tag is skipped, when it's value is zero.

Following keys are added:
Exif.Canon.LensModel
Exif.CanonCs.MaxAperture
Exif.CanonSi.ApertureValue
Exif.MinoltaCs5D.ExposureManualBias
Exif.MinoltaCs5D.MeteringMode
Exif.MinoltaCsNew.MinoltaModel
Exif.MinoltaCsOld.MeteringMode
Exif.MinoltaCsOld.MinoltaModel
Exif.Nikon3.ISOSettings
Exif.NikonLd4.LensID
Exif.NikonLd4.LensIDNumber
Exif.NikonLd4.MaxAperture
Exif.NikonPc.Saturation
Exif.NikonSiD300a.ISO
Exif.Olympus2.Macro
Exif.Olympus2.Quality
Exif.OlympusCs.ExposureMode
Exif.OlympusCs.MeteringMode
Exif.OlympusEq.SerialNumber
Exif.OlympusRd.ExposureBiasValue
Exif.OlympusRd2.ExposureBiasValue
Exif.PanasonicRaw.Make
Exif.PanasonicRaw.Model
Exif.PanasonicRaw.Orientation
Exif.Pentax.ExposureTime
Exif.Pentax.FNumber
Exif.Pentax.Flash
Exif.Pentax.MeteringMode
Exif.Pentax.ModelID
Exif.Pentax.Quality
Exif.Pentax.SerialNumber
Exif.Pentax.WhiteBalance
Exif.PentaxDng.ExposureTime
Exif.PentaxDng.FNumber
Exif.PentaxDng.Flash
Exif.PentaxDng.MeteringMode
Exif.PentaxDng.ModelID
Exif.PentaxDng.Quality
Exif.PentaxDng.SerialNumber
Exif.PentaxDng.WhiteBalance
Exif.Photo.BodySerialNumber
Exif.Sigma.MeteringMode
Exif.Sony1.ExposureMode
Exif.Sony1Cs.MeteringMode
Exif.Sony1Cs2.ExposureProgram
Exif.Sony1Cs2.MeteringMode
Exif.Sony2.ExposureMode
Exif.Sony2.SonyModelID
Exif.Sony2Cs.MeteringMode
Exif.SonyMinolta.Quality
Exif.SonyMisc2b.ExposureProgram
Exif.SonyMisc3c.Quality2

[codecov]

Codecov Report

Merging #2362 (dae3436) into main (d2253c9) will increase coverage by 0.00%.
The diff coverage is 37.50%.

@@           Coverage Diff           @@
##             main    #2362   +/-   ##
=======================================
  Coverage   63.51%   63.52%           
=======================================
  Files         119      119           
  Lines       20624    20631    +7     
  Branches    10233    10238    +5     
=======================================
+ Hits        13100    13106    +6     
  Misses       5395     5395           
- Partials     2129     2130    +1     

Impacted Files	Coverage Δ
src/easyaccess.cpp	89.43% <37.50%> (-3.68%)	⬇️
src/nikonmn_int.cpp	61.67% <0.00%> (+0.21%)	⬆️
src/olympusmn_int.cpp	41.51% <0.00%> (+0.72%)	⬆️

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

[piponazo]

Hi @norbertwg , thanks for your PR.

I see that you have been discussing with @postscript-dev & @kmilos about how to make the changes on this file. I have no experience with easyaccess but so far your changes look good and the CI is happy with them.

You mentioned that you have been analyzing some sample images to validate your changes. I think it would be a good idea to add some of the images to our test suite to verify that your changes work as expected.

Did you explore the area of the code in which we run our tests?
Under test/data/ you can see all the test data files. In order to extract the metadata into an .exv file, you can follow this wiki page:
https://github.com/Exiv2/exiv2/wiki/Creating-a-test-file

And in the folder tests you can see different categories of python tests.

In case you are willing to add some of these tests but you need help to do that, we can help you with it 😸

[norbertwg]

@piponazo Adding some images to the test suite should not be a problem. But as I am not familiar with python, adding tests will be difficult. Give me some days to look into the existing tests. Then I will get back, hopefully with an idea how to procede.

[postscript-dev]

@norbertwg
Exiv2 has a tests/README-TESTS document that explains the Python testing. The main branch version of the doc maybe slightly different to the 0.27-maintenance version. I'm not sure.

[norbertwg]

@postscript-dev thanks, will have a look into it.

[norbertwg]

@postscript-dev
Using tests/README-TESTS I created some tests based on easyaccess.py. In general it was quite easy. But I got a problem when I tried to create a test with file copies. Using the example from the document

import system_tests, CopyTmpFiles
@CopyTmpFiles("$data_path/invalid_input_file.txt")
class AnInformativeName(metaclass=system_tests.CaseMeta):
filename = path("$tmp_path/invalid_input_file.txt")

I got errors. After comparing it with the approach in easyaccess.py I got following approach working:

import system_tests
@system_tests.CopyTmpFiles("$data_path/invalid_input_file.txt")
class AnInformativeName(metaclass=system_tests.CaseMeta):
filename = "$tmp_path/invalid_input_file.txt"

Seems, the documentation needs an update.

easyaccess.py is using CopyFiles. As the documentation suggests CopyTmpFiles, I assume CopyTmpFiles is the preferred way, right?

And another question: When I started with the test cases, I thought it could be a good idea to check if all other existings test cases still run with my modification. To run them some executables are needed, not only exiv2, I am working with Visual Studio and the solution created following the instructions does not build those additional programs. Is there a simple way to create them?

[postscript-dev]

@norbertwg:

Using tests/README-TESTS I created some tests based on easyaccess.py.

Using the example from the document, I got errors.

Seems, the documentation needs an update.

I can find examples (e.g., bash_tests/test_exifprint_lint.py) where the README-TESTS.md copy format is used, so perhaps there was something else wrong. You can run an individual test to check that it is working correctly. For example, when in the exiv2/tests directory, run:

$ python3 runner.py bash_tests/test_exifprint_lint.py
.
----------------------------------------------------------------------
Ran 1 test in 0.151s

OK

easyaccess.py is using CopyFiles. As the documentation suggests CopyTmpFiles, I assume CopyTmpFiles is the preferred way, right?

I think that CopyTmpFiles is more recent and is preferred instead of CopyFiles. This is because CopyTmpFiles preserves the copied file whereas CopyFiles automatically deletes it at the end of the test. Preserving the temporary file helps debugging.

You only need to copy a file if you are modifying it. If you are treating the test files are read-only then copying is better removed.

And another question: When I started with the test cases, I thought it could be a good idea to check if all other existings test cases still run with my modification. To run them some executables are needed, not only exiv2, I am working with Visual Studio and the solution created following the instructions does not build those additional programs. Is there a simple way to create them?

By default, exiv2 does not build the sample programs which are used by some of the tests. To fix this, open CMakeLists.txt and set EXIV2_BUILD_SAMPLES to ON. I also set EXIV2_BUILD_UNIT_TESTS to ON to include the unit tests. There are further notes in README.md.

On a side note, unless you need a particular file format, it is better to add files in the 'raw metadata' format (.exv). Such files only store the metadata (in a JPEG format) and are much smaller than normal image files. There are examples of extracting files to the 'raw metadata' format in exiv2.md.

[norbertwg]

@ postscript-dev
Thanks a lot for your fast and detailed response.
I made the suggested changes in CMakeLists.txt and built again as described in README.md. Now "python3 runner.py" completed with status OK.

Then I had a look into bash_tests/test_exifprint_lint.py. The import there is
from system_tests import CaseMeta, CopyTmpFiles, path
which looks different from the example in tests/README-TESTS:
import system_tests, CopyTmpFiles
I intend to follow the example in bash_tests/test_exifprint_lint.py to be aligned and because - as you mentioned - preserving temporary files helps debugging.

Regarding exv: So far I could use existing files for the test cases. In my manual tests I executed before committing the changes in easyaccess.cpp, I had to use some RAW files. Adding an exv instead of a RAW file will make huge differenze in size, so for sure I will do it.

[neheb]

Needs a rebase.

[norbertwg]

@ShawnBaker
As I am rather new here, can you please explain what actions are required to "rebase" and who should do it?

[postscript-dev]

@norbertwg

As I am rather new here, can you please explain what actions are required to "rebase" and who should do it?

Rebasing involves updating your git branch with the new changes that have since been made in exiv2's branch (i.e., exiv2:main). Rebasing isn't needed in every PR but is here. This is because the lines of code that you are trying to change have since been changed and this causes a conflict. Although there are only a few minor differences, this needs to be resolved before merging can take place.

If git can rebase automatically, then this is a couple of simple commands. However if not automatic, then an interactive rebase is needed - which is a little more work. You would normally be responsible for rebasing but if you are stuck, then let us know.

Usually in your fork of exiv2, you create a new branch (e.g., norbertwg:update_easyAccessAPI based from norbertwg:main) to store all the changes that you make. I see that you are using the norbertwg:main branch for your changes and I am not sure if this will still work. @kevinbackhouse , is a new branch needed?

Exiv2 has some more general information in GIT_GUIDELINES.md.

[kevinbackhouse]

I think the merge conflict is because src/easyaccess.cpp was recently modified in #2323. The merge looks non-trivial because both PRs modified the lists of keys.

I would probably do this one manually, with a diff-tool like meld. I.e. create two clones of Exiv2. In the first clone, checkout norbertwg:main. In the second, checkout out the latest exiv2:main. Using meld, manually copy the changes from the first clone to the second. Then create a new branch in the second clone and start a fresh PR (or do a force push to this PR if you know how to do that).

[norbertwg]

@postscript-dev @kevinbackhouse
Thanks for your fast support. My impression is as well, that the merge is non-trivial and at least not a good exercise for me to try git's automatic rebase for the first time. So I will merge it manually. Then I will look for test images to enhance the test cases I created in the meantime, create a branch and a fresh PR - as I do not know how to do force push.