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.

Sign up for GitHub

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

Jump to bottom

Fix 1343 png 0.27 #1351

Merged
merged 3 commits into from
Oct 11, 2020
Merged

Fix 1343 png 0.27 #1351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
806ffac
Adding test files and bash script/reference-output
clanmills Oct 9, 2020
834dca3
Code changes
clanmills Oct 9, 2020
9fa6d39
C++ changes requested by @piponazo. Fix python png_test() recommende…
clanmills Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 37 additions & 13 deletions src/pngimage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ namespace Exiv2 {
bool zTXt = std::strcmp(chType,"zTXt")== 0;
bool iCCP = std::strcmp(chType,"iCCP")== 0;
bool iTXt = std::strcmp(chType,"iTXt")== 0;
bool eXIf = std::strcmp(chType,"eXIf")== 0;

// for XMP, ICC etc: read and format data
bool bXMP = option == kpsXMP && findi(dataString,xmpKey)==0;
Expand All @@ -313,7 +314,7 @@ namespace Exiv2 {
bool bSoft = option == kpsRecursive && findi(dataString,softKey)==0;
bool bComm = option == kpsRecursive && findi(dataString,commKey)==0;
bool bDesc = option == kpsRecursive && findi(dataString,descKey)==0;
bool bDump = bXMP || bICC || bExif || bIptc || bSoft || bComm || bDesc ;
bool bDump = bXMP || bICC || bExif || bIptc || bSoft || bComm || bDesc || eXIf ;

if( bDump ) {
DataBuf dataBuf;
Expand All @@ -339,6 +340,9 @@ namespace Exiv2 {
if ( iTXt ) {
bGood = (start+3) < dataOffset ; // good if not a nul chunk
}
if ( eXIf ) {
bGood = true ;// eXIf requires no pre-processing)
}

// format is content dependent
if ( bGood ) {
Expand Down Expand Up @@ -383,6 +387,11 @@ namespace Exiv2 {
out.write((const char*)decoded.pData_,decoded.size_);
bLF = true;
}
if ( eXIf && option == kpsRecursive ) {
// create memio object with the data, then print the structure
BasicIo::AutoPtr p = BasicIo::AutoPtr(new MemIo(data,dataOffset));
printTiffStructure(*p,out,option,depth);
}

if ( bLF ) out << std::endl;
}
Expand Down Expand Up @@ -448,8 +457,11 @@ namespace Exiv2 {

/// \todo analyse remaining chunks of the standard
// Perform a chunk triage for item that we need.
if (chunkType == "IEND" || chunkType == "IHDR" || chunkType == "tEXt" || chunkType == "zTXt" ||
chunkType == "iTXt" || chunkType == "iCCP") {
if(chunkType == "IEND" || chunkType == "IHDR"
|| chunkType == "tEXt" || chunkType == "zTXt"
|| chunkType == "eXIf"
|| chunkType == "iTXt" || chunkType == "iCCP"
){
DataBuf chunkData(chunkLength);
readChunk(chunkData, *io_); // Extract chunk data.

Expand All @@ -463,6 +475,13 @@ namespace Exiv2 {
PngChunk::decodeTXTChunk(this, chunkData, PngChunk::zTXt_Chunk);
} else if (chunkType == "iTXt") {
PngChunk::decodeTXTChunk(this, chunkData, PngChunk::iTXt_Chunk);
} else if (chunkType == "eXIf") {
ByteOrder bo = TiffParser::decode(exifData(),
iptcData(),
xmpData(),
chunkData.pData_,
chunkData.size_);
setByteOrder(bo);
} else if (chunkType == "iCCP") {
// The ICC profile name can vary from 1-79 characters.
uint32_t iccOffset = 0;
Expand Down Expand Up @@ -540,9 +559,9 @@ namespace Exiv2 {
// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

if (io_->error()) throw Error(kerFailedToReadImageData);
if (bufRead != cheaderBuf.size_) throw Error(kerInputDataReadFailed);
if (bufRead != 8) throw Error(kerInputDataReadFailed);

// Decode chunk data length.

Expand All @@ -551,7 +570,7 @@ namespace Exiv2 {

// Read whole chunk : Chunk header + Chunk data (not fixed size - can be null) + CRC (4 bytes).

DataBuf chunkBuf(8 + dataOffset + 4); // Chunk header (8 bytes) + Chunk data + CRC (4 bytes).
DataBuf chunkBuf(8 + dataOffset + 4); // Chunk header (8 bytes) + Chunk data + CRC (4 bytes).
memcpy(chunkBuf.pData_, cheaderBuf.pData_, 8); // Copy header.
bufRead = io_->read(chunkBuf.pData_ + 8, dataOffset + 4); // Extract chunk data + CRC
if (io_->error()) throw Error(kerFailedToReadImageData);
Expand All @@ -561,16 +580,21 @@ namespace Exiv2 {
memcpy(szChunk,cheaderBuf.pData_ + 4,4);
szChunk[4] = 0;

if (!memcmp(cheaderBuf.pData_ + 4, "IEND", 4))
if ( !strcmp(szChunk,"IEND") )
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 for replacing the memcmp functions by strcmp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

{
// Last chunk found: we write it and done.
#ifdef EXIV2_DEBUG_MESSAGES
std::cout << "Exiv2::PngImage::doWriteMetadata: Write IEND chunk (length: " << dataOffset << ")\n";
#endif
if (outIo.write(chunkBuf.pData_, chunkBuf.size_) != chunkBuf.size_) throw Error(kerImageWriteFailed);
if (outIo.write(chunkBuf.pData_, chunkBuf.size_) != chunkBuf.size_)
throw Error(kerImageWriteFailed);
return;
}
else if (!memcmp(cheaderBuf.pData_ + 4, "IHDR", 4))
else if ( !strcmp(szChunk, "eXIf") ) {
; // do nothing Exif metdata is written following IHDR
; // as zTXt chunk with signature Raw profile type exif__
}
else if ( !strcmp(szChunk, "IHDR") )
{
#ifdef EXIV2_DEBUG_MESSAGES
std::cout << "Exiv2::PngImage::doWriteMetadata: Write IHDR chunk (length: " << dataOffset << ")\n";
Expand Down Expand Up @@ -672,10 +696,10 @@ namespace Exiv2 {
}
}
}
else if (!memcmp(cheaderBuf.pData_ + 4, "tEXt", 4) ||
!memcmp(cheaderBuf.pData_ + 4, "zTXt", 4) ||
!memcmp(cheaderBuf.pData_ + 4, "iTXt", 4) ||
!memcmp(cheaderBuf.pData_ + 4, "iCCP", 4))
else if (!strcmp(szChunk, "tEXt") ||
!strcmp(szChunk, "zTXt") ||
!strcmp(szChunk, "iTXt") ||
!strcmp(szChunk, "iCCP"))
{
DataBuf key = PngChunk::keyTXTChunk(chunkBuf, true);
if (compare("Raw profile type exif", key, 21) ||
Expand Down
2 changes: 2 additions & 0 deletions test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ TESTS = addmoddel.sh \
iso65k-test.sh \
modify-test.sh \
path-test.sh \
png-test.sh \
preview-test.sh \
stdin-test.sh \
stringto-test.sh \
Expand All @@ -102,6 +103,7 @@ icc-test \
iso65k-test \
modify-test \
path-test \
png-test \
stringto-test \
webp-test \
write2-test \
Expand Down
Binary file added test/data/1343_comment.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/data/1343_empty.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/data/1343_exif.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
71 changes: 71 additions & 0 deletions test/data/png-test.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
STRUCTURE OF PNG FILE: 1343_empty.png
address | chunk | length | data | checksum
8 | IHDR | 13 | .......d.... | 0x4ce4e85c
33 | sRGB | 1 | | 0xaece1ce9
46 | gAMA | 4 | .... | 0x0bfc6105
62 | pHYs | 9 | ......... | 0xc76fa864
83 | IDAT | 257 | x^..1..0......t.....h......z.. | 0x64465429
352 | IEND | 0 | | 0xae426082

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
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
changed comment
STRUCTURE OF PNG FILE: 1343_comment.png
address | chunk | length | data | checksum
8 | IHDR | 13 | .......d.... | 0x4ce4e85c
33 | sRGB | 1 | | 0xaece1ce9
46 | gAMA | 4 | .... | 0x0bfc6105
62 | pHYs | 9 | ......... | 0xc76fa864
83 | IDAT | 257 | x^..1..0......t.....h......z.. | 0x64465429
352 | tEXt | 24 | Comment.Can you read me? | 0x83cad5fa
388 | IEND | 0 | | 0xae426082

STRUCTURE OF PNG FILE: 1343_comment.png
address | chunk | length | data | checksum
8 | IHDR | 13 | .......d.... | 0x4ce4e85c
33 | iTXt | 39 | Description.....x.K.H.KOMQH... | 0xe0a1c8ce
84 | sRGB | 1 | | 0xaece1ce9
97 | gAMA | 4 | .... | 0x0bfc6105
113 | pHYs | 9 | ......... | 0xc76fa864
134 | IDAT | 257 | x^..1..0......t.....h......z.. | 0x64465429
403 | tEXt | 24 | Comment.Can you read me? | 0x83cad5fa
439 | IEND | 0 | | 0xae426082
changed comment
STRUCTURE OF PNG FILE: 1343_exif.png
address | chunk | length | data | checksum
8 | IHDR | 13 | .......d.... | 0x4ce4e85c
33 | sRGB | 1 | | 0xaece1ce9
46 | gAMA | 4 | .... | 0x0bfc6105
62 | pHYs | 9 | ......... | 0xc76fa864
83 | IDAT | 257 | x^..1..0......t.....h......z.. | 0x64465429
352 | eXIf | 108 | MM.*.................J........ | 0x764e86e1
472 | IEND | 0 | | 0xae426082

Exif.Image.ImageDescription Ascii 17 Can you read me?
Exif.Image.XResolution Rational 1 72
Exif.Image.YResolution Rational 1 72
Exif.Image.ResolutionUnit Short 1 inch
Exif.Image.YCbCrPositioning Short 1 Centered
STRUCTURE OF PNG FILE: 1343_exif.png
address | chunk | length | data | checksum
8 | IHDR | 13 | .......d.... | 0x4ce4e85c
33 | iTXt | 39 | Description.....x.K.H.KOMQH... | 0xe0a1c8ce
84 | zTXt | 145 | Raw profile type exif..x.U.... | 0x2f6d89e8
241 | sRGB | 1 | | 0xaece1ce9
254 | gAMA | 4 | .... | 0x0bfc6105
270 | pHYs | 9 | ......... | 0xc76fa864
291 | IDAT | 257 | x^..1..0......t.....h......z.. | 0x64465429
560 | IEND | 0 | | 0xae426082
changed comment
Exif.Image.ImageDescription Ascii 17 Can you read me?
Exif.Image.XResolution Rational 1 72
Exif.Image.YResolution Rational 1 72
Exif.Image.ResolutionUnit Short 1 inch
Exif.Image.YCbCrPositioning Short 1 Centered
23 changes: 23 additions & 0 deletions test/png-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#!/usr/bin/env bash
# Test PNG support (-pS, -pc, -pa -c )

source ./functions.source

( cd "$testdir"

files="1343_empty.png 1343_comment.png 1343_exif.png"
copyTestFiles $files
for file in $files ; do
for i in 1 2 ; do
runTest exiv2 -pS $file
runTest exiv2 -pc $file
runTest exiv2 -pa $file
runTest exiv2 -c 'changed comment' $file
done
done
) > $results 2>&1

reportTest

# That's all Folks!
##
32 changes: 32 additions & 0 deletions tests/bash_tests/testcases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,38 @@ def path_test(self):
# print(out)


def png_test(self):
# Test PNG support (-pS, -pc, -c, -pa)
# files="1343_empty.png 1343_comment.png 1343_exif.png"
# copyTestFiles $files
# runTest exiv2 -c 'comment in Reagan to hide non UTF-8 bytes'
# for file in $files ; do
# for i in 1 2 ; do
# runTest exiv2 -pS $file
# runTest exiv2 -pc $file
# runTest exiv2 -pa $file
# runTest exiv2 -c 'changed comment' $file
# done
# done

out = BT.Output()
files = ['1343_empty.png'
,'1343_comment.png'
,'1343_exif.png'
]
for file in files:
BT.copyTestFile(file)
for i in [1,2]:
out += BT.Executer('exiv2 -pS {file}', vars())
out += BT.Executer('exiv2 -pc {file}', vars())
if i ==1:
out+= ''
out += BT.Executer('exiv2 -pa {file}', vars())
out += BT.Executer('exiv2 -c "changed comment" {file}', vars())

BT.reportTest('png-test', out) # skip! blank lines from -pc are in the wrong order!


def preview_test(self):
# Test driver for previews
images = [
Expand Down