Fix 1343 png 0.27 #1351

clanmills · 2020-10-09T05:50:44Z

See #1343. This change deals with the eXIf chunks in PNG. It does not change how exiv2 deals with PNG "Textual information".

@LeoHsiao1 There's an issue with bash_tests/testcases.py png_test(). It's ignoring a blank line in the output from the command exiv2 -pc test/data/1343-empty.png (and a couple of other blank lines). I ran the code in the Visual Code Debugger. Your knowledge of python is way beyond mine, so haven't understood how BT.Executer handles stdout. Beautiful code. Very neat and well laid out.

piponazo · 2020-10-09T08:19:46Z

src/pngimage.cpp

@@ -533,16 +552,17 @@ namespace Exiv2 {
        // Write PNG Signature.
        if (outIo.write(pngSignature, 8) != 8) throw Error(kerImageWriteFailed);

-        DataBuf cheaderBuf(8);       // Chunk header : 4 bytes (data size) + 4 bytes (chunk type).
+        DataBuf cheaderBuf(9);   // Chunk header : 4 bytes (data size) + 4 bytes (chunk type) + nul


I do not understand why we need to do this change.

You are changing the size from 8 to 9 but then the last byte is never used.
In line 556 you are setting byte[8] to 0, but that is already done in line 562 where we set the whole byte array to 0.

Could you explain why this is needed?

I did this to use strcmp() instead of memcmp() to inspect different chunks. You can revert that, if you prefer memcmp.()

In my opinion, I think this change is not needed . We are passing szChunk to the strcmp function and not cheaderBuf. You can still copy 4 bytes from cheaderBuf to szChunk and szChunk can have the 5th byte set to NULL.

piponazo · 2020-10-09T08:20:50Z

src/pngimage.cpp


        while(!io_->eof())
        {
            // Read chunk header.

            std::memset(cheaderBuf.pData_, 0x00, cheaderBuf.size_);
-            long bufRead = io_->read(cheaderBuf.pData_, cheaderBuf.size_);
+            long bufRead = io_->read(cheaderBuf.pData_, 8);


In case we additional byte is not needed at the end, I would revert this change and use cheaderBuf.size_ instead of 8 (which could be considered as a magic value)

piponazo · 2020-10-09T08:22:26Z

src/pngimage.cpp

@@ -561,16 +581,21 @@ namespace Exiv2 {
            memcpy(szChunk,cheaderBuf.pData_ + 4,4);
            szChunk[4]  = 0;

-            if (!memcmp(cheaderBuf.pData_ + 4, "IEND", 4))
+            if ( !strcmp(szChunk,"IEND") )


👍 for replacing the memcmp functions by strcmp

Would you like me to revert some of this stuff and update the PR?

nono, the comment was positive, I think it is more readable to use strcmp for the string comparisons rather than the original memcp 😉

LeoHsiao1 · 2020-10-09T11:36:45Z

I compiled fix_1343_png_0.27, but executed the command incorrectly:

leo@Leo:/mnt/d/1/exiv2/test/tmp$ /mnt/d/1/exiv2/build/bin/exiv2 -pR 1343_empty.png
exiv2: Action not available in Release mode: 'R'
Usage: exiv2 [ options ] [ action ] file ...

Manipulate the Exif metadata of images.

Am I using the wrong compiled version of exiv2?

leo@Leo:/mnt/d/1/exiv2/test/tmp$ /mnt/d/1/exiv2/build/bin/exiv2 -V
exiv2 0.27.3

clanmills · 2020-10-09T12:33:06Z

Ah, yes. You're right. That option is only valid on a debug build. The default build on macOS is Debug. I'll have to change that (probably to -pS).

Can you change it to -pS on your local copy and that will enable you to reproduced the "missing empty line" puzzle which which I'd like to investigate.

Luis has asked me to modify the C++ code in the PR. I'll try to deal with all of this today.

LeoHsiao1 · 2020-10-09T13:10:31Z

BT.Executer removes the blank line at the end of stdout by default. Since this is irrelevant to the test case, you can add it manually:

            for i in [1, 2]:
                out  += BT.Executer('exiv2 -pS    {file}', vars())
                out  += ''
                out  += BT.Executer('exiv2 -pc    {file}', vars())
                out  += BT.Executer('exiv2 -pa    {file}', vars())
                out  += BT.Executer('exiv2 -c "changed comment" {file}', vars())

BTW, In your reference output, changed comment should appear in STRUCTURE OF PNG FILE? Isn't this a mess?

STRUCTURE OF PNG FILE: 1343_empty.png
 address | chunk |  length | data                           | checksum
       8 | IHDR  |      13 | .......d....                   | 0x4ce4e85c
      33 | iTXt  |      39 | Description.....x.K.H.KOMQH... | 0xe0a1c8ce
changed comment
      84 | sRGB  |       1 |                                | 0xaece1ce9
      97 | gAMA  |       4 | ....                           | 0x0bfc6105
     113 | pHYs  |       9 | .........                      | 0xc76fa864
     134 | IDAT  |     257 | x^..1..0......t.....h......z.. | 0x64465429
     403 | IEND  |       0 |                                | 0xae426082

clanmills · 2020-10-09T13:36:02Z

Thanks for getting back so quickly. Is this really the best fix? No possibility of getting BT.Executer to retain the empty line?

out  += ''

You're right, it is a "bit of mess". The options -pS, -pR were added for debugging files and that's why they are only build in debug mode. When it's debugging a PNG, it prints the Comment from the iTXt chunk. Yesterday, I thought about changing -pR to skip printing the Comment. I decided it to leave it as Luis would ask "why is this being changed?" in the review.

There's an option $ exiv2 -c abcdefg foo.jpg which will set the "Comment" in a JPEG file. You can print the comment with $ exiv2 -pc foo. A "Comment" in a JPEG is a top level "segment" in the JPEG. Somebody decided to use those commands on a PNG to update an iTXt chunk with the signature "Description". I don't think that was a good idea. However it's been this way for a long time, so I'll leave it alone.

I've rediscovered lots of stuff I had forgotten about PNG files (thanks to #1343). I'm adding that to the book now. Then I'll make the C++ changes that Luis has requested.

clanmills · 2020-10-09T13:41:47Z

Ah, that's fine. You'll see our young Chinese collaborator @LeoHsiao1 is working on the python "bash_tests" for this.

@LeoHsiao1 say hello to @piponazo (Luis). @piponazo say hello to @LeoHsiao1 (whom I call Leo).

LeoHsiao1 · 2020-10-09T13:54:46Z

Most test cases do not require empty lines at the end, such as:

    def modify_test(self):
        # Test driver for write unit tests to build Exif metadata from scratch
        for i in ['exiv2-empty.jpg', 'exiv2-gc.jpg', 'modifycmd1.txt', 'modifycmd2.txt']:
            BT.copyTestFile(i)
        out  = BT.Output()
        out += BT.Executer('exiv2 -v -m modifycmd1.txt exiv2-empty.jpg')
        out += BT.Executer('exiv2 -v -m modifycmd2.txt exiv2-gc.jpg')
        out += BT.Executer('exiv2 -v -pi exiv2-empty.jpg')
        out += BT.Executer('exiv2 -v -pt exiv2-empty.jpg exiv2-gc.jpg')
        BT.reportTest('modify-test', out)

When I encounter an unexpected blank line in the output file, I always make sure it does not affect the content of the test, and then manually fix it. Although this adds some ugly code, it's at least easy to understand.

@piponazo Hello, I guess it's dinner time for you guys, and I'm about to go to bed.
My Chinese name is Hsiao, and my English name is Leo. A long time ago, I couldn't tell the difference between English surnames and first names.

piponazo · 2020-10-09T14:13:50Z

@LeoHsiao1 Hi Leo! Nice to have a first contact with you. Here it is not even dinner time yet, still on work time 😭 .

clanmills · 2020-10-09T14:18:19Z

I'm going to stick to calling you Leo. You're a Lion! Luis lives in Switzerland and is Spanish. Luis is a bull-fighter when he's not fixing code! It's coming to the end of the working week for us. Sleep Well. Thank You for working on the project.

I'll add the out += '' to png_test() in tests/bash_tests/testcases.py .

When I was a boy in Scotland (60 years ago), we used to tell the joke:

Q Why are there no phones in Honk Kong?
A There are so many Wings and so many Wongs, the Wings would Wing the Wong Number!

(helps to know that when Chinese people say R, it sounds to an English speaker like W. When my Chinese friend Gee calls, he says: "Herro Wobin".)

@LeoHsiao1

… by @LeoHsiao1. Update reference output.

clanmills · 2020-10-11T09:15:15Z

@LeoHsiao1 I thought of a great pun. @piponazo at the week-end is a bull-fighter. Monday to Friday, he's a bool-fighter!

I recently watched the movie "The Genius of George Boole" on Amazon Prime Videos. Very interesting. https://www.imdb.com/title/tt6517392/

LeoHsiao1 · 2020-10-11T11:42:43Z

Let me guess. Bool-fighter means fighting against binary Numbers?

clanmills · 2020-10-11T12:04:51Z

Something like that. bool is a keyword in the C++ language (but not in C). https://www.sitesbay.com/cpp/cpp-keywords

I believe George Boole sorted out the mathematics of true and false. So, he formulated operators such as OR, AND, NOT, XOR and possibly bit-shift operators as well. Perhaps he discovered that numbers could be represented in bits. Somebody had to discover that.

The movie talks about the importance of his discoveries without explaining what he discovered. At least it didn't explain it in a way that left me certain about what he did. https://en.wikipedia.org/wiki/George_Boole

We've enjoyed several vacations in Spain and never been to a bull fight.

clanmills added 2 commits October 9, 2020 06:39

Adding test files and bash script/reference-output

806ffac

Code changes

834dca3

clanmills requested review from piponazo and LeoHsiao1 October 9, 2020 05:50

clanmills mentioned this pull request Oct 9, 2020

Test Localisation #1278

Closed

piponazo reviewed Oct 9, 2020

View reviewed changes

C++ changes requested by @piponazo. Fix python png_test() recommended…

9fa6d39

… by @LeoHsiao1. Update reference output.

piponazo approved these changes Oct 11, 2020

View reviewed changes

clanmills merged commit bc01aac into 0.27-maintenance Oct 11, 2020

clanmills deleted the fix_1343_png_0.27 branch October 11, 2020 09:15

kevinbackhouse added this to the v0.27.4 milestone Nov 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 1343 png 0.27 #1351

Fix 1343 png 0.27 #1351

clanmills commented Oct 9, 2020

piponazo Oct 9, 2020

clanmills Oct 9, 2020

piponazo Oct 9, 2020

piponazo Oct 9, 2020

piponazo Oct 9, 2020

clanmills Oct 9, 2020

piponazo Oct 9, 2020

LeoHsiao1 commented Oct 9, 2020 •

edited

Loading

clanmills commented Oct 9, 2020

LeoHsiao1 commented Oct 9, 2020

clanmills commented Oct 9, 2020 •

edited

Loading

clanmills commented Oct 9, 2020 •

edited

Loading

LeoHsiao1 commented Oct 9, 2020 •

edited

Loading

piponazo commented Oct 9, 2020

clanmills commented Oct 9, 2020

clanmills commented Oct 11, 2020

LeoHsiao1 commented Oct 11, 2020

clanmills commented Oct 11, 2020

Fix 1343 png 0.27 #1351

Fix 1343 png 0.27 #1351

Conversation

clanmills commented Oct 9, 2020

piponazo Oct 9, 2020

Choose a reason for hiding this comment

clanmills Oct 9, 2020

Choose a reason for hiding this comment

piponazo Oct 9, 2020

Choose a reason for hiding this comment

piponazo Oct 9, 2020

Choose a reason for hiding this comment

piponazo Oct 9, 2020

Choose a reason for hiding this comment

clanmills Oct 9, 2020

Choose a reason for hiding this comment

piponazo Oct 9, 2020

Choose a reason for hiding this comment

LeoHsiao1 commented Oct 9, 2020 • edited Loading

clanmills commented Oct 9, 2020

LeoHsiao1 commented Oct 9, 2020

clanmills commented Oct 9, 2020 • edited Loading

clanmills commented Oct 9, 2020 • edited Loading

LeoHsiao1 commented Oct 9, 2020 • edited Loading

piponazo commented Oct 9, 2020

clanmills commented Oct 9, 2020

clanmills commented Oct 11, 2020

LeoHsiao1 commented Oct 11, 2020

clanmills commented Oct 11, 2020

LeoHsiao1 commented Oct 9, 2020 •

edited

Loading

clanmills commented Oct 9, 2020 •

edited

Loading

clanmills commented Oct 9, 2020 •

edited

Loading

LeoHsiao1 commented Oct 9, 2020 •

edited

Loading