Address issue #2083, fixing a time-zone bug in a unit test

[mallman]

Address issue #2083, fixing a time-zone related bug in test_issue_1959.py.

Note, I'm following the documentation in the exiv2.md in setting the TZ variable.

[codecov]

Codecov Report

Merging #2084 (e845953) into main (4ae42b1) will decrease coverage by 0.21%.
The diff coverage is n/a.

❗ Current head e845953 differs from pull request most recent head 5e1129d. Consider uploading reports for the commit 5e1129d to get more accurate results

@@            Coverage Diff             @@
##             main    #2084      +/-   ##
==========================================
- Coverage   62.08%   61.87%   -0.22%     
==========================================
  Files          96       96              
  Lines       19153    19059      -94     
  Branches     9798     9782      -16     
==========================================
- Hits        11891    11792      -99     
- Misses       4967     4984      +17     
+ Partials     2295     2283      -12     

Impacted Files	Coverage Δ
src/nikonmn_int.cpp	46.76% <ø> (ø)
src/safe_op.hpp	69.23% <0.00%> (-27.65%)	⬇️
include/exiv2/slice.hpp	69.64% <0.00%> (-21.89%)	⬇️
include/exiv2/error.hpp	60.71% <0.00%> (-4.81%)	⬇️
src/utils.cpp	38.46% <0.00%> (-1.93%)	⬇️
include/exiv2/value.hpp	82.68% <0.00%> (-0.56%)	⬇️
src/enforce.hpp	75.00% <0.00%> (+2.77%)	⬆️
src/getopt.cpp	67.30% <0.00%> (+5.76%)	⬆️
include/exiv2/metadatum.hpp	88.88% <0.00%> (+16.16%)	⬆️

---

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 4ae42b1...5e1129d. Read the comment docs.

[mallman]

Derp. Guess this patch isn't going to work so great on Windows. I'll work on a more robust approach.

[hassec]

@piponazo I think this conflicts with your #2079

@mallman thanks a lot for filing the issue and directly working on it 👍
I do like where your solution is going, but wonder if we can somehow fix the timezone globally for the entire testsuite.

[piponazo]

Hehe it is funny that we ended up trying to fix the same issue on the same day. Another wonder of the FOSS community 😇 .

I do not believe this is the first case where such problem arised. @clanmills Maybe you can guide us on this topic. Did you ever encountered such problems? (The one described at #2083)

[mallman]

Actually, I'm putting this on hold while I investigate a closely related matter.

[mallman]

I've put this PR on hold, because I'm not sure this test is well-defined. Why is the system's timezone (or value of TZ) relevant in determining Exif.Photo.DateTimeOriginal in this test case?

According to the documentation of Exif.Photo.DateTimeOriginal on https://exiv2.org/tags.html, Exif.Photo.DateTimeOriginal is "The date and time when the original image data was generated." To emphasize, it is not a date and time, it is the date and time.

If I shot a photo at 6 AM on 1 January, 2000 in Iceland, shouldn't Exif.Photo.DateTimeOriginal be "2000:01:01 06:00:00"? And if I shot a photo at 6 AM on 1 January, 2000 in Namibia, shouldn't Exif.Photo.DateTimeOriginal also be "2000:01:01 06:00:00"?

Why would my system timezone matter? In both cases, it is the local time zone that determines the value of Exif.Photo.DateTimeOriginal.

And in this particular test case, the only creation date we have is "2022-01-04T09:41:01+00:00". How do we compute the value of Exif.Photo.DateTimeOriginal given that? In examining the source document for this test, I could find no information on the local time zone.

[clanmills]

@mallman The TZ environment string should not be involved with Exif.Photo.DateTimeOriginal. It's an ascii string in the metadata. The Exif Specification added tags for TZ and milliseconds and these tags are supported in Exiv2.

653 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ taglist all | grep Original | grep Photo
Photo.DateTimeOriginal
Photo.OffsetTimeOriginal
Photo.SubSecTimeOriginal
654 rmills@rmillsm1:~/gnu/github/exiv2/main/build $

Exiv2 should not post-process that metadata in any way. It don't see any evidence that it does. I will inspect the code.

656 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ env TZ=GMT exiv2 -Pv -K Exif.Photo.DateTimeOriginal ~/Stonehenge.jpg 
2015:07:16 15:38:54
657 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ env TZ=PST8PDT exiv2 -Pv -K Exif.Photo.DateTimeOriginal ~/Stonehenge.jpg 
2015:07:16 15:38:54
658 rmills@rmillsm1:~/gnu/github/exiv2/main/build $ 

Thank You for opening #2082. I've discussed this with @piponazo and @kmilos and will review/update README.md and CONTRIBUTING.md over the weekend. I haven't heard if @hassec did service in the Swiss Army.

[mallman]

It's an ascii string in the metadata.

In this particular unit test Exif.Photo.DateTimeOriginal is not present in the source data. For one thing, there is no original image with embedded exif data—everything comes from an XMP document, which itself does not define Exif.Photo.DateTimeOriginal. I don't know how the exiv2 tool uses the information in an XMP document to derive Exif.Photo.DateTimeOriginal when it isn't present. My concern/question is around whether such information can even be recovered in this case.

[clanmills]

@mallman Apologies. I hadn't looked at the test when I wrote about Exif.Photo.DateTimeOriginal.

I now understand that this is an XMP test. There is date information in that file:

713 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ xmllint --pretty 2 issue_1959_poc.xmp | grep -i date
        photoshop:DateCreated="2022-01-04T09:41:01+00:00"
                Iptc4xmpExt:AOCircaDateCreated="2022-01-04"
                Iptc4xmpExt:AODateCreated="2022-01-04T09:41:01+00:00"
714 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ 

That's being "converted" into Exif.Photo.DateTimeOriginal:

713 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ xmllint --pretty 2 issue_1959_poc.xmp | grep -i date
        photoshop:DateCreated="2022-01-04T09:41:01+00:00"
                Iptc4xmpExt:AOCircaDateCreated="2022-01-04"
                Iptc4xmpExt:AODateCreated="2022-01-04T09:41:01+00:00"
714 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ exiv2 -pe issue_1959_poc.xmp
Exif.Image.YCbCrPositioning                  Short       1  1
Exif.Image.XResolution                       Rational    1  72/1
Exif.Image.YResolution                       Rational    1  72/1
Exif.Image.ResolutionUnit                    Short       1  2
Exif.Image.ImageDescription                  Ascii      32  Test file for the IPTC XMP tags
Exif.Image.Artist                            Ascii      15  postscript-dev
Exif.Image.Copyright                         Ascii      16  Copyright Exiv2
Exif.Photo.ExifVersion                       Undefined   4  48 50 51 50
Exif.Photo.FlashpixVersion                   Undefined   4  48 49 48 48
Exif.Photo.ColorSpace                        Short       1  65535
Exif.Photo.ComponentsConfiguration           Undefined   1  1
Exif.Photo.DateTimeOriginal                  Ascii      20  2022:01:04 09:41:01
715 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ 

In the dungeons of Exiv2, there are metadata "convertors" what attempt to synchronise data between different standards (Exif, Iptc and Xmp). I don't know who thought of this bad idea.

I've just changed the timezone on my MacBook Pro to the West Coast (I lived in San Jose for 15 years). And shock/horror:

715 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ exiv2 -pe issue_1959_poc.xmp > ~/temp/England.txt
... use SysPrefs to move to California ...
717 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ date
Thu 10 Feb 2022 11:02:58 PST
718 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ exiv2 -pe issue_1959_poc.xmp > ~/temp/California.txt
719 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ diff ~/temp/England.txt ~/temp/California.txt 
12c12
< Exif.Photo.DateTimeOriginal                  Ascii      20  2022:01:04 09:41:01
---
> Exif.Photo.DateTimeOriginal                  Ascii      20  2022:01:04 01:41:01
720 rmills@rmillsm1:~/gnu/github/exiv2/main/test/data $ 

A lunatic is using a TZ time conversion function. That's obviously wrong. Wrong, wrong, wrong. Shsshssssss.

I don't need the output I request before. I can easily generate it:

724 rmills@rmillsm1:~/gnu/github/exiv2/main/tests $ python3 runner.py bugfixes/github/*1959*.py 2>&1 | wc -l
     250
725 rmills@rmillsm1:~/gnu/github/exiv2/main/tests $ python3 runner.py bugfixes/github/*1959*.py 
...
  Xmp.photoshop.CaptionWriter                   Test Name  Test Name
  Xmp.dc.title                                  lang="x-default" Test IPTC XMP file  lang="x-default" Test IPTC XMP file
  Xmp.dc.subject                                Test  Test
  Xmp.dc.creator                                postscript-dev  postscript-dev
  Xmp.dc.rights                                 lang="x-default" Copyright Exiv2  lang="x-default" Copyright Exiv2
  Xmp.dc.description                            lang="x-default" Test file for the IPTC XMP tags  lang="x-default" Test file for the IPTC XMP tags
 : Standard output does not match

----------------------------------------------------------------------
Ran 1 test in 0.046s

FAILED (failures=1)
       0       0       0
724 rmills@rmillsm1:~/gnu/github/exiv2/main/tests $

[mallman]

@clanmills Would you agree with me that the output of exiv2 in this case should not include the Exif.Photo.DateTimeOriginal tag?

[clanmills]

Again, I've only just discovered your observations. More work ahead for Team Exiv2.

[mallman]

I've raised my concerns around the inference of xmp timestamp data into DateTimeOriginal. (It's somewhat ironic that timestamp conversion even arose in this issue because the intent of the unit test in question is not to test timestamp conversion.) It's been a week since I opened this PR and we seem to have a working solution. In the interests of keeping on point, I'm going to take this PR out of draft and request a merge from a reviewer.

Questions around timestamp/local time conversion can be raised and discussed in a separate, focused issue another time. Cheers!

[mallman]

test_issue_1959.py was deleted in b318b9d, rendering this PR moot. Closing without merge.

[clanmills]

@mallman. Thanks for this. Gosh, you're a clever guy to have spotted out. Thanks.

I think I will restore test_issue_1959.py and submit a PR in which I remove the photoshop entry in the side-car. However, I'll do this when I understand why test_issue_1959.py was deleted.

Thanks also for your PR about Nikon/AF2. What you've done seems right to me, however I don't want to get sucked into that issue.

[piponazo]

I think I will restore test_issue_1959.py and submit a PR in which I remove the photoshop entry in the side-car. However, I'll do this when I understand why test_issue_1959.py was deleted.

Hi Robin. That test was removed in #2091 (you can see the discussion there)

[clanmills]

@mallman I could really enjoy working with you. Yes. I think removing the 1959 tests in #2091 has something to do with babies and bathwater (or in my case drowning the grandkids in the swimming pool). I'm going to submit a PR which I think will be accepted without using more of your time. Thanks very much.

[clanmills]

#2103