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

Initialize a non-empty PNG ICC profile name #2355

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kmilos
Copy link
Collaborator

@kmilos kmilos commented Sep 15, 2022

Fixes #2354

@kmilos kmilos added bug imageHandler Anything related to specific ImageHandlers labels Sep 15, 2022
@kmilos kmilos force-pushed the fix_png_empty_icc_name branch from 10d190c to 7715493 Compare September 15, 2022 09:23
@kevinbackhouse
Copy link
Collaborator

Fix looks good. Do we have a test image for this? (I don't see one attached to #2354 either.)

@kevinbackhouse
Copy link
Collaborator

I think conan must have made a backwards-incompatible change that's broken our build. I got the same failures in #2353.

@kmilos
Copy link
Collaborator Author

kmilos commented Sep 15, 2022

Fix looks good.

I might add the possibility to set the name though (API only, not CLI), as mentioned in the issue...

Do we have a test image for this?

We do have one test that seems to produce an empty profile name:

def stdin_test(self):
return # temporarily disable
# Test driver for stdin
try:
import lxml
except ModuleNotFoundError:
print('Skipped. Because it misses module lxml. Please install: `pip install lxml`')
return
out = BT.Output()
a = 'exiv2-bug1229.jpg' # jpg with 2 APP1/xap segments
b = 'girl.jpg'
BT.copyTestFile(a)
def get_xmpData(img):
e = BT.Executer('exiv2 -pX {img}', vars(), decode_output=False)
return e.stdout.replace(b'\n', b'\r\n', 16) # Ignore the difference in newline
BT.copyTestFile(a, b)
out += BT.Executer('exiv2 -pS {b}', vars())
out += BT.Executer('exiv2 -dX {b}', vars()) # remove first
out += BT.Executer('exiv2 -pS {b}', vars())
e = BT.Executer('exiv2 -pX {a}', vars(), decode_output=False)
with open('out2', 'wb') as f:
f.write(e.stdout)
out += BT.Executer('exiv2 -iXX- {b}', vars(), stdin=get_xmpData(a))
out += BT.Executer('exiv2 -pS {b}', vars())
BT.copyTestFile(a, b)
out += BT.Executer('exiv2 -dX {b}', vars())
out += BT.Executer('exiv2 -dX {b}', vars())
out += BT.Executer('exiv2 -pS {b}', vars())
out += BT.Executer('exiv2 -iXX- {b}', vars(), stdin=get_xmpData(a))
out += BT.Executer('exiv2 -pS {b}', vars())
for f in ['Reagan.jpg', 'Reagan.tiff', 'ReaganSmallPng.png']:
BT.copyTestFile(f)
out += BT.Executer('exiv2 -iXX- {f}', vars(), stdin=get_xmpData(a))
e = BT.Executer('exiv2 -pX {f}', vars())
out += """
<?xml version="1.0"?>
<?xpacket begin='\ufeff' id='W5M0MpCehiHzreSzNTczkc9d'?>
{}
<?xpacket end='w'?>
""".strip('\n').format(BT.pretty_xml(e.stdout))
for f in ['Reagan.jpg', 'ReaganSmallPng.png']:
BT.copyTestFile(f)
BT.copyTestFile(a, b)
out += BT.Executer('exiv2 -pS {b}', vars())
e = BT.Executer('exiv2 -ea- {f}', vars(), decode_output=False)
out += BT.Executer('exiv2 -ia- {b}', vars(), stdin=e.stdout)
out += BT.Executer('exiv2 -pS {b}', vars())
BT.copyTestFile('Reagan.tiff') # 1272 ReaganLargeTiff.tiff
for img in ['Reagan.jpg', 'ReaganSmallPng.png', 'exiv2-bug1199.webp']:
BT.copyTestFile(img)
e = BT.Executer('exiv2 -eC- Reagan.tiff', decode_output=False)
# Ignore the difference in the path separator
stdout = e.stdout
for pair in [
(b'\x03/\x9e', b'\x03\\\x9e'),
(b'\x0c/\x0c', b'\x0c\\\x0c'),
(b'V/V' , b'V\\V' ),
(b'\xe5/5' , b'\xe5\\5' ),
(b'5/\x86' , b'5\\\x86' ),
(b'\x86/\xd6', b'\x86\\\xd6'),
(b'\xac/\xac', b'\xac\\\xac'),
(b'\xd7/\xd7', b'\xd7\\\xd7'),
]:
stdout = stdout.replace(pair[0], pair[1])
out += BT.Executer('exiv2 -iC- {img}', vars(), stdin=stdout)
out += BT.Executer('exiv2 -pS {img}', vars())
if img == 'ReaganSmallPng.png':
with open('out2', 'wb') as f:
f.write(e.stdout)
BT.reportTest('stdin-test', out)

But it should have failed now, no? Needs looking into...

@kmilos
Copy link
Collaborator Author

kmilos commented Sep 15, 2022

Ah, the stdin_test above is disabled 🤦

When I enable it, it does fail for many other reasons, but in the generated test/tmp/stdin-test.out I see the corrected

    9337 | iCCP  |    2604 | ICC Profile..x...wTS.....7.&.. | 0x876f2b5d

instead of the current, empty, test/data/test_reference_files/stdin-test.out reference:

    9337 | iCCP  |    2598 | ..x...wTS.....7.P.....khR.H..H | 0x915a222e

@kmilos kmilos marked this pull request as draft September 15, 2022 13:28
@kmilos
Copy link
Collaborator Author

kmilos commented Sep 15, 2022

Actually, specialization of both setICCProfile() and clearICCProfile() is mandatory now that I thought about it more...

@kmilos kmilos marked this pull request as ready for review September 19, 2022 10:27
@kmilos kmilos force-pushed the fix_png_empty_icc_name branch from 2cc3b0a to 673fdb7 Compare September 19, 2022 10:45
@kmilos kmilos force-pushed the fix_png_empty_icc_name branch from 673fdb7 to e7cefa3 Compare September 19, 2022 13:18
@kmilos kmilos marked this pull request as draft January 19, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted imageHandler Anything related to specific ImageHandlers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using setIccProfile on a PNG file without a profile leads to an invalid (empty) profile name
2 participants