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

Add Exif Gamma tag #1548

Merged
merged 2 commits into from
Apr 12, 2021
Merged

Add Exif Gamma tag #1548

merged 2 commits into from
Apr 12, 2021

Conversation

kmilos
Copy link
Collaborator

@kmilos kmilos commented Apr 12, 2021

No description provided.

clanmills
clanmills previously approved these changes Apr 12, 2021
Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

Several comments:

  1. Do you have a test file and python script?
  2. Are there other missing Exif Tags from Table 7 of https://fotomagazin.hu/wp-content/uploads/2020/05/CIPA_DC_008_EXIF_2019.pdf
  3. What do you think about adding the tags in Support for DNG 1.5 #1397 to this PR?
  4. Is printFloat() the correct prettyPrinter? Do you know how ExifTool presents this data.? I think it's between 0.0 and 1.0 If in doubt ask @alexvanderberkel because he knows about Photography.

@clanmills
Copy link
Collaborator

I see this has died a horrible (and well deserved) death in the test suite. You'll have to update that. For example:

/Users/travis/build/Exiv2/exiv2/test/data/exiv2-test.out: 8756 lines
863/Users/travis/build/Exiv2/exiv2/test/tmp/exiv2-test.out: 8756 lines
864The first mismatch is in line 979:
865< 20040329_224245.jpg   Exif.Photo.0xa500                            Rational    1  22/10
866> 20040329_224245.jpg   Exif.Photo.Gamma                             Rational    1  2.2

It's easy to fix. You'll have to edit test/data/exiv2-test.out and replace >0xa500< with >Gamma <. Watch out for the space following Gamma.

@clanmills
Copy link
Collaborator

When the test suite has been fixed, we don't need to bother adding anything to the python tests (in tests/bugfixes/github). It's already tested by exiv2-test (which is in tests/bash_tests). It's is called 'bash_tests' for historical reasons. Leo rewrote them in python and that make them much easier to run on Visual Studio builds.

@kmilos
Copy link
Collaborator Author

kmilos commented Apr 12, 2021

Sorry, I'll fix up the test data, I wrongly assumed there was nothing there...

For the other questions:

  1. Are there other missing Exif Tags from Table 7

Nope, this was the only one. I checked tables 7, 8, and 15.

  1. What do you think about adding the tags in Support for DNG 1.5 #1397 to this PR?

We have all the DNG tags in up to 1.6 already in 0.27.4-RC2. What's missing in #1397 is "only" support for separate DCP (DNG Camera Profile) IFDs and standalone ".dcp" TIFF-like files... I think you tested these in your tvisitor and covered it in your book, but we don't support them in exiv2 yet...

  1. Is printFloat() the correct prettyPrinter? Do you know how ExifTool presents this data.?

It's not suitable for all rationals for sure. The main question for the ones where it does make sense is whether we want to print units as well or not... I'll check what ExifTool does as well for some of these.

Let's keep that discussion to the other issue report, and handle this PR for Gamma first, and maybe back-port this only missing tag to 0.27.4?

@clanmills
Copy link
Collaborator

Thanks. I'm happy to see this merged. The existing way (prior to the change) is:

632 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -g a500 --unknown test/data/exiv2-nikon-d70.jpg
Exif.Photo.0xa500                            Rational    1  22/10
633 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance $ 

It's so much nicer with your PR:

641 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ exiv2 -g gamma/i ../test/data/exiv2-nikon-d70.jpg
Exif.Photo.Gamma                             Rational    1  2.2
642 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ 

And that "rhymes" well with ExifTool:

642 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ exiftool ../test/data/exiv2-nikon-d70.jpg | grep -i gamma
Gamma                           : 2.2
643 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ 

I'm not going to back-port it to 0.27-maintenance. After RCfinal, I only want to change the revision number and update documentation. I avoid C++ changes. It's the same reason that I don't want to add Exiv2::enableBMFF() without EXV_ENABLE_BMFF.

If we must have an RC3 (for a security fix or darktable reports a bmff issue), I can back-port this PR to 0.27-maintenance.

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

Good Job, @kmilos. Thanks for dealing with this.

@kmilos
Copy link
Collaborator Author

kmilos commented Apr 12, 2021

Sure, that release policy makes sense.

Thanks.

@kmilos kmilos merged commit 6cb3402 into main Apr 12, 2021
@kmilos kmilos deleted the add_exif_gamma branch April 12, 2021 17:35
kmilos added a commit that referenced this pull request Jul 1, 2021
Add Exif Gamma tag

(cherry picked from commit 6cb3402)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants