Skip to content

Lens: Olympus M.Zuiko Digital ED 17mm F1.2 Pro (0.27 only) #1375

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

Merged

Conversation

olupton
Copy link
Contributor

@olupton olupton commented Oct 20, 2020

The title pretty much says it all!

I tested with a RAW file taken using this lens, without this change exiv2 -pt output includes:

Exif.OlympusEq.LensType                      Byte        6  0 0 41 16 0 0

with this patch it shows:

Exif.OlympusEq.LensType                      Byte        6  Olympus M.Zuiko Digital ED 17mm F1.2 Pro

I took the exact wording of the lens name from the 25mm equivalent just above.

@clanmills
Copy link
Collaborator

Two comments:

1 Can you add test a test file and test script for this, please. Here's an example from a user yesterday #1373. I explained the test harness to him here: #1368 (comment)

2 We have two development branches - '0.27-maintenance' and 'master'. I maintain 0.27-maintenance branch. If you focus your PR on 0.27-maintenance, it will be ported to 'master' at release time. You may wish to submit separate PRs.

@olupton olupton force-pushed the add-olympus-m.zuiko-17mm-f1.2-pro branch from 53a4151 to b5c0058 Compare October 20, 2020 19:36
@olupton olupton changed the base branch from master to 0.27-maintenance October 20, 2020 19:37
@olupton olupton changed the title Add LensType entry for Olympus M.Zuiko Digital ED 17mm F1.2 Pro lens Add LensType entry for Olympus M.Zuiko Digital ED 17mm F1.2 Pro lens (0.27->master) Oct 20, 2020
@olupton
Copy link
Contributor Author

olupton commented Oct 20, 2020

Thanks for the feedback!

I have addressed both points, this PR now targets 0.27 and I added a simple test.

(For future readers: I used the following series of commands to create the test file:

exiv2 -ee --force ${name}.JPG
exiv2 -dt ${name}.exv
exiv2 \
  -M'del Exif.OlympusEq.SerialNumber' \
  -M'del Exif.OlympusEq.InternalSerialNumber' \
  -M'del Exif.OlympusEq.LensSerialNumber' \
  -M'del Exif.OlympusEq.ExtenderSerialNumber' \
  -M'del Exif.OlympusEq.FlashSerialNumber' \
  ${name}.exv
mv ${name}.exv olympus-m.zuiko-17mm-f1.2-pro.exv

I hope this is a useful reference.)

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.

Thank You for your contribution and Thank You for a quick and complete response to my review.

This looks right. I'll test it tomorrow to:

  1. To be sure it works as you say.
  2. To compare existing and modified behaviour.
  3. To be sure you have not broken an existing test.

I expect to merge it into 0.27-maintenance on Wednesday, unless somebody else deal with it first!.

@clanmills clanmills self-assigned this Oct 20, 2020
@clanmills clanmills added the lens Issue related to lens detection label Oct 20, 2020
@clanmills clanmills added this to the v0.27.4 milestone Oct 20, 2020
@clanmills
Copy link
Collaborator

clanmills commented Oct 21, 2020

These is something amiss here involving git/github/branch. You'll see that appveyor (the Visual Studio CI) growls: AppVeyor was unable to build non-mergeable pull request. Probably the quick way to fix this is to open a new PR on 0.27-maintenance. I could easily do this for you, however I want you to get the credit for the fix.

There are lots of grumbles from exiv2-test.sh and I'm sure that's due to changes that are not in your base. I've built as follows:

619 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ git pull --rebase
...
620 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ cd build
621 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ curl -L --silent https://github.com/Exiv2/exiv2/pull/1375.patch | git apply
622 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/build $ make ; make tests
...

The change here is:

625 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 --grep lens/i test/data/olympus-m.zuiko-17mm-f1.2-pro.exv 2>/dev/null
Exif.OlympusEq.LensType                      Byte        6  0 0 41 16 0 0
Exif.OlympusEq.LensModel                     Ascii      32  OLYMPUS M.17mm F1.2 <--- Was
Exif.OlympusEq.LensFirmwareVersion           Long        1  4097
Exif.OlympusEq.LensProperties                Short       1  16640
Exif.OlympusEq.ConversionLens                Ascii      32  
Exif.Photo.LensSpecification                 Rational    4  17/1 17/1 12/10 12/10
Exif.Photo.LensModel                         Ascii      32  OLYMPUS M.17mm F1.2
626 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ build/bin/exiv2 --grep lens/i test/data/olympus-m.zuiko-17mm-f1.2-pro.exv 2>/dev/null
Exif.OlympusEq.LensType                      Byte        6  Olympus M.Zuiko Digital ED 17mm F1.2 Pro <--- Changed
Exif.OlympusEq.LensModel                     Ascii      32  OLYMPUS M.17mm F1.2
Exif.OlympusEq.LensFirmwareVersion           Long        1  4097
Exif.OlympusEq.LensProperties                Short       1  16640
Exif.OlympusEq.ConversionLens                Ascii      32  
Exif.Photo.LensSpecification                 Rational    4  17/1 17/1 12/10 12/10
Exif.Photo.LensModel                         Ascii      32  OLYMPUS M.17mm F1.2
627 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ 

The following metadata has not changed. This is correct. It's a 32 byte Ascii string written by the camera.

Exif.Photo.LensModel                         Ascii      32  OLYMPUS M.17mm F1.2

Minor point about your test script. You don't need to specify -pa and -K. This is benign. If you create a new PR, you'll want to modify tests/bugfixes/github/test_pr_1375.py anyway.

I think the quickest way to finish this is for you to create a new PR. You may know another way to calm the CI.

@piponazo
Copy link
Collaborator

Thanks @olupton for your work here.

I have created a wiki page with the snippet you added here explaining how to create a .exv file:
https://github.com/Exiv2/exiv2/wiki/Create-test-file

@clanmills feel free to review the content of the page a modify it you feel something is missing or my English is not appropriate 😛 .

As @clanmills mentioned, It seems like the CI did not re-run things after changing the base branch from master to 0.27-maintenance. I will close the PR and try to re-open it. In case that is not possible, @olupton please re-open it.

@piponazo piponazo closed this Oct 21, 2020
@piponazo
Copy link
Collaborator

Reopening PR with the aim of re-triggering the CI jobs.

@piponazo piponazo reopened this Oct 21, 2020
@piponazo
Copy link
Collaborator

piponazo commented Oct 21, 2020

I also added the label to-master so that mergify creates an automatic PR trying to port these changes to master (after this PR is merged)

@clanmills
Copy link
Collaborator

This all looks fine, @piponazo. The CI has been retriggered by the close/open and has succeeded.

When the Rule: forward-port patches to master (backport) succeeds, we can merge this, and modify title of the PRs to avoid future confusion (probably 0.27 only, and master only).

@clanmills clanmills merged commit 21a0501 into Exiv2:0.27-maintenance Oct 22, 2020
@clanmills clanmills changed the title Add LensType entry for Olympus M.Zuiko Digital ED 17mm F1.2 Pro lens (0.27->master) Add LensType entry for Olympus M.Zuiko Digital ED 17mm F1.2 Pro lens (0.27 only) Oct 22, 2020
@clanmills clanmills changed the title Add LensType entry for Olympus M.Zuiko Digital ED 17mm F1.2 Pro lens (0.27 only) Lens: Olympus M.Zuiko Digital ED 17mm F1.2 Pro (0.27 only) Oct 22, 2020
@clanmills
Copy link
Collaborator

clanmills commented Oct 25, 2020

@piponazo I've given your Wiki Page a little english polish.

It's a good idea to use the Wiki pages for subjects that are frequently discussed. I've added another page about adding a new lens and referenced yours: https://github.com/Exiv2/exiv2/wiki/Adding-a-new-lens

@clanmills clanmills mentioned this pull request Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lens Issue related to lens detection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants