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

Fix write ability flags of PSD files #1260

Merged
merged 1 commit into from
Aug 10, 2020
Merged

Fix write ability flags of PSD files #1260

merged 1 commit into from
Aug 10, 2020

Conversation

tbeu
Copy link
Member

@tbeu tbeu commented Aug 10, 2020

No description provided.

@tbeu tbeu requested a review from clanmills August 10, 2020 17:11
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.

I wonder why those flags are set to AmRead? psdimage.cpp does have a writeMetadata() function. Is it possible to add some tests to be sure this is working?

There is only one photoshop image in the test suite that's not related to a POC (security) or an old issue and that is used in preview-test.sh. It would be very desirable to actually test reading and writing Exif/XMP/IPTC/ICC metadata.

1449 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/test $ ls -l data/*.psd
-rw-r--r--+ 1 rmills  staff  64164 11 May 16:08 data/exiv2-photoshop.psd
1450 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/test $ grep exiv2-photoshop *.sh
preview-test.sh:        exiv2-photoshop.psd \
1451 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/test $ 

I have documented the PSD format in my book. https://clanmills.com/exiv2/book/#PSD

@clanmills clanmills added this to the v0.27.4 milestone Aug 10, 2020
@tbeu
Copy link
Member Author

tbeu commented Aug 10, 2020

Indeed, I also wondered. In https://dev.exiv2.org/projects/exiv2/wiki/Supported_image_formats PSD is listed as r/w, too.

@tbeu
Copy link
Member Author

tbeu commented Aug 10, 2020

Is it possible to add some tests to be sure this is working?

Is it as simple as updating exifdata-test.sh with that file exiv2-photoshop.psd?

@clanmills
Copy link
Collaborator

I think this stuff works. It's documented W/R everywhere. As we don't have anything in the test suite, there are two possibilities:

1 Those settings are not respected by the code! <--- This is the case
2 Nobody has ever tried to write to a PhotoShop file! <-- No way to know!

1463 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ cp test/data/exiv2-photoshop.psd .
1464 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -g DateTime exiv2-photoshop.psd 
Exif.Image.DateTime                          Ascii      20  2011:06:27 21:41:02
1465 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -M'set Exif.Image.DateTime 2020:08:10 18:41:42' exiv2-photoshop.psd 
1466 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ exiv2 -g DateTime exiv2-photoshop.psd 
Exif.Image.DateTime                          Ascii      20  2020:08:10 18:41:42
1467 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ 

I checked the log for src/psdimage.cpp

1458 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/src $ git log psdimage.cpp | tail
Author: Andreas Huggel <ahuggel@gmx.net>
Date:   Thu May 1 08:27:35 2008 +0000

    s/Ovolabs/Ovolab/

commit 654d51a366c8bc8fb74473198ef9d58a048cc776
Author: Andreas Huggel <ahuggel@gmx.net>
Date:   Tue Apr 15 03:46:01 2008 +0000

    Added read support for jp2 and psd images, stubs for gif, bmp and tga images, and pixelWidth and pixelHeight methods on Image (Marco Piovanelli).
1459 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/src $ git log psdimage.cpp | grep write
    #606: Added Michael Ulbrich's patch for Exif and IPTC write-support.
    #606: Added IPTC write support for PSD images (Patch from Michael Ulbrich)
1460 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance/src $

So PSD support was added in April 2008. I joined the project in August 2008.

Patch 606 by Michael Ulbrich is mentioned in my book. It deals very thoroughly will several edge conditions concerning IPTC in JPEG files (and other horrors). I suspect the PSD support in Exiv2 is strong thanks to those patches. https://dev.exiv2.org/issues/606

I'm happy to approve this PR as it is benign (does no harm). However, I would like to see some tests added to the test suite. Can you open a new issue for that please and set Milestone: 0.27.4. I'm working hard on the book at the moment, and don't want to divert into this topic at the moment.

BTW, I have an engineer in China working with my on #1215 and he has an open PR #1257. I'm hoping he will complete his PR next Sunday. Once that's done, I'd like him to test our Charset code #1258. I believe it works correctly, however I'd like a native speaker of a language which requires UNICODE (or JIS) to test it. Maybe, after that, I could ask him to work on the testing photoshop files in Exiv2.

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.

I'm happy to approve this PR as it is benign (does no harm).

@tbeu tbeu merged commit f61fa2e into 0.27-maintenance Aug 10, 2020
@tbeu tbeu deleted the fix-psd-type branch August 10, 2020 18:08
@clanmills
Copy link
Collaborator

clanmills commented Aug 10, 2020

Updating the test suite. Simply update exiv2-test.sh? Well, "sort of". The Chinese engineer is converting bash-tests to python. It's Work-in-progress, so better to leave this until he's finished.

Dan really wants us to totally discontinue the bash-tests and adopt his tests/systems_test.py framework for all future testing. I have agreed to that. So, we should really add new tests using Dan's model.

However, I proposed #1215 as a quick and easy way to execute bash_tests without using bash!

When the Chinese Engineer has finished working on #1257, we can use either approach. I suspect modifying the equivalent of "exiv2-test` in python is the quick and simple way to deal with this.

Dan objected very strongly to #1215. Please understand that I like Dan's system_tests.py. It's very modular. However when you have a "family test" such as exiv2-test.sh or icctest-sh, it's quick and easy to add more files to an existing test. By "family test" i mean something like Exif or XMP or ICC. The same test is run on a collection of files.

We could add functionality to system_tests.py to run the same test on a collection of files. That's effectively what I've asked the Chinese Engineer to do.

@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants