Tests fail with ImageMagick compiled with --with-quantum-depth=8 or 32 #120
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I think it's better to open a separate issue for that. I don't really understand how that setting manages to affect tests. I had the same suspicion as you but I think (not 100% sure) that I tested
identifyagainst the same PNG file and it always showed the right depth. I decided that I would just prevent tests from running with something other than the default quantum depth of 16 and that was it.Does the test suite at some point use ImageMagick to create new files? In that case, maybe this setting affects the depth of the created files?
I am attaching logs from builds with both variants. I noticed that the quantum depth appears in the version string: ImageMagick 7.1.0-4 Q32 x86_64 2021-07-18.
The Q8 version seems to fail a lot of tests whereas the Q32 is not as bad.
Yes, I'm using the
converttool from imagemagick to create most of the test input. That's why I'm checking the bit depth everywhere, to be sure that the test input I create is the input I expect it to be.That makes sense, because there are a lot of tests that expect imagemagick to handle 16bit depth which will probably fail with Q8 but work fine with Q32.
I can confirm that the
depthattribute contains the compile-time constant quantum depth for TIFF images whereasbaseDepthcontains the actual depth (but for PNG or JPG, both attributes contain the actual depth, go figure 😕). What we should probably do is drop all lines wheredepthis checked, replacing it withbaseDepthif it is not checked.This way, tests would pass with Q32 (I think the failures on Q8 should just be ignored, I doubt anyone will have IM built with it and if they want working tests, they just need to skip those that require 16 bit depth).
What is the minimum IM version you want to support (in other words, what Debian/Ubuntu version)? If I change the tests, I want to make sure that they don't break on older versions.
Well tests fail with Python 3.5 because of a str/bytes thing so I guess the minimum version will be Focal with 3.6.
Unless I'm very much mistaken, Focal ships Python 3.8
My bad, I meant Bionic.
Yes, that sounds reasonable.
I'm a Debian user which currently only ships 6.9.11.60. I have no idea what the current status of the transition to version 7 is, which would then also land in Ubuntu. I'm a Debian Developer and can help the packaging effort and offered my help in the relevant bug report: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929825
Personally I have little issue with just requiring IM7 and call it a day. This would also push me to put some work into finally having IM7 in Debian (and thus later also Ubuntu). But I guess too many people will be using old Ubuntu LTS versions for a while so I fear that we will continue being stuck on having to support IM 6.9.7.4 which is the version in Ubuntu Bionic, the last supported LTS.
Whoops, clicked the close button by mistake.
Updating ImageMagick to version 7 in Debian (and subsequently in Ubuntu) would be nice. I'm having some strange issues converting svg to ico that apparently got fixed with IM7 (https://github.com/cfcurtis/pdfstitcher/pull/96, https://github.com/cfcurtis/pdfstitcher/pull/101).
Good to know, in that case maybe the next Ubuntu version will have IM 7 at least.
I tried replacing depth with baseDepth everywhere but it breaks some tests. Not all TIFF files behave the same way so I opened an IM issue: https://github.com/ImageMagick/ImageMagick/issues/4109.
@josch I don't know if you followed the issue I opened but it sounds like you should indeed stop testing the value of
depth.@sbraz do you have imagemagick 7 on gentoo? I just installed 7.1.0.5 and adapted the tests so that they work with IM7. I'm pondering to drop IM6 support because of all the differences in the output depending on the minor version.
@josch I do have IM7 and with the commits I pushed to main, the tests pass fine. Are you seeing additional failures?
Which commits are you refering to?
Currently, with IM7, the tests pass fine for me locally. Unfortunately, this breaks the Travis tests because those still use Ubuntu which only has IM6 available. But I don't have enough time to maintain compatibility with multiple IM versions, so I thought I just make it work with the lastest and call it a day.
I meant all the commits from #117
Those made the tests work with both IM6 and IM7. I am surprised that tests fail on Travis but I am pretty sure this is unrelated to the changes because they failed before I started adding commits for IM7.
I have probably provided example Dockerfiles in other issues to run the test suite on Debian Bullseye and other OSes but I don't remember everything now.
I don't mind you dropping support for older IM versions but I'm not sure how Debian/Ubuntu users will feel about this change.
Luckily, I'm the Debian maintainer of img2pdf so I can adjust things there as needed.
To come back to the original topic of this issue: Do you also have patches for img2pdf to run the tests with imagemagick with a quantum depth other than 16?
Well if you can make Debian switch to IM7, that'd be cool :) I just don't realise how much work that represents.
I do not. I don't think it is worth the trouble since Q8 and Q32 builds seem rare. Q8 will probably not work as expected with most tests so I'd skip it.
As for Q32, you can probably store the value of the build's quantum-depth and reuse it in tests. From what the IM maintainer explained, I'm not sure if the behaviour we're seeing might change some day. Maybe we should skip the bit depth tests for the problematic files, I don't really know :/
@sbraz, did
10c6901fa3maybe also fix this one?Hi @josch. It's been a while since I last used the package. I'll try to look into it.