I have a stack of png file to read. They appear fine in Preview and various other image handling programs. In Squeak (both 5.3 release and the trunk, on both ARM and intel VMs) they look rather odd - almost as if every other line is missing.
From Preview
Squeak 6 trunk arm
Squeak 6 mac intel update 22101
same mac, squeak6 trunk
.. but those last two are artefacts of the size of the FileList and resizing it larger - so the preview in the bottom pane gets larger - returns the displayed image to match the arm system. Evidently the resizing is a bit messed up too. Ah, and on a Pi resizing the FileList does the same things, so at least we're platform consistently messed up!
I have plenty of other png files that appear perfectly ok in Squeak and rescale cleanly.
Before I start whacking at things in a debugger I thought I'd ask if anyone else plays with PNGs right now and has been fixing things.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful random insult:- Wasn't fully debugged before being released.
Can you share one of the misbehaving PNGs?
I know there’s a bug somewhere that manifests in decoding PNGs. I thought it was specific to the SqueakJS VM though:
https://github.com/codefrau/SqueakJS/issues/23
Vanessa
On Sat, Feb 25, 2023 at 10:55 tim Rowledge tim@rowledge.org wrote:
I have a stack of png file to read. They appear fine in Preview and various other image handling programs. In Squeak (both 5.3 release and the trunk, on both ARM and intel VMs) they look rather odd - almost as if every other line is missing.
From Preview
Squeak 6 trunk arm Squeak 6 mac intel update 22101 same mac, squeak6 trunk
.. but those last two are artefacts of the size of the FileList and resizing it larger - so the preview in the bottom pane gets larger - returns the displayed image to match the arm system. Evidently the resizing is a bit messed up too. Ah, and on a Pi resizing the FileList does the same things, so at least we're platform consistently messed up!
I have plenty of other png files that appear perfectly ok in Squeak and rescale cleanly.
Before I start whacking at things in a debugger I thought I'd ask if anyone else plays with PNGs right now and has been fixing things.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful random insult:- Wasn't fully debugged before being released.
On 2023-02-25, at 12:06 PM, Vanessa Freudenberg vanessa@codefrau.net wrote:
Can you share one of the misbehaving PNGs?
D'Oh, of course -
That's the same one.
I know there’s a bug somewhere that manifests in decoding PNGs. I thought it was specific to the SqueakJS VM though:
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim "Bother" said Pooh, as the IRS kicked his door in.
On Sat, Feb 25, 2023 at 1:52 PM tim Rowledge tim@rowledge.org wrote:
Hah! Shows up yellow in SqueakJS:
[image: image.png]
That's just like that bug report from 8 years ago! Many (most?) PNGs are not afflicted with this issue, so I wonder what's different. There is a tiny test PNG in the bug report.
Vanessa
I know there’s a bug somewhere that manifests in decoding PNGs. I thought it was specific to the SqueakJS VM though:
https://github.com/codefrau/SqueakJS/issues/23
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim "Bother" said Pooh, as the IRS kicked his door in.
Ohhh would you look at that ... works perfectly in a 2.8 image:
[image: image.png]
Vanessa
On Sat, Feb 25, 2023 at 2:36 PM Vanessa Freudenberg vanessa@codefrau.net wrote:
On Sat, Feb 25, 2023 at 1:52 PM tim Rowledge tim@rowledge.org wrote:
Hah! Shows up yellow in SqueakJS:
[image: image.png]
That's just like that bug report from 8 years ago! Many (most?) PNGs are not afflicted with this issue, so I wonder what's different. There is a tiny test PNG in the bug report.
Vanessa
I know there’s a bug somewhere that manifests in decoding PNGs. I thought it was specific to the SqueakJS VM though:
https://github.com/codefrau/SqueakJS/issues/23
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim "Bother" said Pooh, as the IRS kicked his door in.
On 2023-02-25, at 2:41 PM, Vanessa Freudenberg vanessa@codefrau.net wrote:
Ohhh would you look at that ... works perfectly in a 2.8 image:
Oh, right, so we've now written advanced new technology bugs. Lovely! These problem files are from CCTV cameras so who knows how well they obey any standards.
The current PNGReadWriter has a debug output to the Transcript that tells me the image has - form = Form(252x172x32) colorType = 2 interlaceMethod = 1 filters = {137->1 . 129->3 . 54->2 . 3->4}
whereas all the PNG's I can find that load 'properly' seem to have colorType = 6, interlace method = 0 and a bit oddly, appear to repeat the debug output as if it is processed twice? Hmm, shomething wrong offisher? Still, a different interlace code might be a good clue given the image actually *looks* like interlace-screwed.
Oh, so much for that theory, I found a demo png with colorType = 3 and interlace 1. Poo.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful Latin Phrases:- Mellita, domi adsum. = Honey, I'm home.
On further testing:
up to 4.4: fine https://squeak.js.org/run/#zip=https://files.squeak.org/4.4/Squeak4.4-12327....
since 4.5: broken https://squeak.js.org/run/#zip=https://files.squeak.org/4.5/Squeak4.5-13680....
Filing 4.4's PNGReadWriter into 4.5 fixes the yellowing issue.
Just filing in 4.4's PNGReadWriter>>copyPixelsRGB: also fixes the yellowing issue.
Attaching ...
(I realize this may be entirely unrelated to the issue you are seeing)
Vanessa
On Sat, Feb 25, 2023 at 3:37 PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-25, at 2:41 PM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
Ohhh would you look at that ... works perfectly in a 2.8 image:
Oh, right, so we've now written advanced new technology bugs. Lovely! These problem files are from CCTV cameras so who knows how well they obey any standards.
The current PNGReadWriter has a debug output to the Transcript that tells me the image has - form = Form(252x172x32) colorType = 2 interlaceMethod = 1 filters = {137->1 . 129->3 . 54->2 . 3->4}
whereas all the PNG's I can find that load 'properly' seem to have colorType = 6, interlace method = 0 and a bit oddly, appear to repeat the debug output as if it is processed twice? Hmm, shomething wrong offisher? Still, a different interlace code might be a good clue given the image actually *looks* like interlace-screwed.
Oh, so much for that theory, I found a demo png with colorType = 3 and interlace 1. Poo.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful Latin Phrases:- Mellita, domi adsum. = Honey, I'm home.
On 2023-02-25, at 3:46 PM, Vanessa Freudenberg vanessa@codefrau.net wrote:
On further testing:
up to 4.4: fine https://squeak.js.org/run/#zip=https://files.squeak.org/4.4/Squeak4.4-12327....
since 4.5: broken https://squeak.js.org/run/#zip=https://files.squeak.org/4.5/Squeak4.5-13680....
Filing 4.4's PNGReadWriter into 4.5 fixes the yellowing issue.
Just filing in 4.4's PNGReadWriter>>copyPixelsRGB: also fixes the yellowing issue.
Attaching ...
(I realize this may be entirely unrelated to the issue you are seeing)
Weird that affects the yellow (R+G) channel but not the blue; and sadly, no it doesn't affect my problem despite being commented as related to color type 2.
An annoying thing here is that I had to look into this code several years ago and I have horrible memories bubbling up about how painful it all was. I really hope it isn't a change I made that is the problem...
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim "How many Kzin does it take to change a lightbulb?” "None. You can scream and leap in the dark."
On 2023-02-25, at 3:46 PM, Vanessa Freudenberg vanessa@codefrau.net wrote:
On further testing:
Some more info
A png that loads nicely has an IHDR chunk, and IDAT chunk and an IEND.
A CCTV png that looks crummy has an unrecognised sRGB chunk (we keep a list in PNGReadWriter>>#processNextChunk and I de-commented the Transcript show of the list in PNGReadWriter>>#nextImage)
A sample png (http://www.libpng.org/pub/png/img_png/pass7_sml.png) really does some oddness - in a 5.3 image it displays without the R+G channels in the FileList preview but is fine if opened as a SketchMorph - in a trunk image it displays perfectly well in both cases - but it has an unrecognised tEXt chunk
Both the tExt & sRGB are legitimate according to the standard but I doubt this can be the source of my current problem if an old 2.8 image can load the image properly.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim If you think C++ is not overly complicated, just what is a protected abstract virtual base pure virtual private destructor and when was the last time you needed one? -- Tom Cargin
On 2023-02-25, at 4:33 PM, tim Rowledge tim@rowledge.org wrote:
Both the tExt & sRGB are legitimate according to the standard but I doubt this can be the source of my current problem if an old 2.8 image can load the image properly.
Hmph. The standard 'suggests' that if a sRGB chunk is present (which it is in my problem images) then one should ignore the gAMA chunk, which we don't, except that we do nothing with it anyway. Like the pHYs chunk my image has, apparently.
Right now the decoded image looks very like the last pass of the interlaced data is simply not dealt with and we end up with the almost-good result. Digging into the actual pixels we seem to end up with row 1: alternating0 & sensible pixel value row 2: sensible pixels values row 3: alternating etc etc
Looking at https://www.w3.org/TR/png/#4Concepts.EncodingScanlineAbs it seems to me that the common issue here might be the pixels shown as being dealt with in pass 6; see the 8 by 8 pattern grid they show. Maybe?
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Computers are only human.
I don't see any problems with this PNG on Windows in an updated trunk image.
Best, Karl
On Sat, Feb 25, 2023 at 11:37 PM Vanessa Freudenberg vanessa@codefrau.net wrote:
On Sat, Feb 25, 2023 at 1:52 PM tim Rowledge tim@rowledge.org wrote:
Hah! Shows up yellow in SqueakJS:
[image: image.png]
That's just like that bug report from 8 years ago! Many (most?) PNGs are not afflicted with this issue, so I wonder what's different. There is a tiny test PNG in the bug report.
Vanessa
I know there’s a bug somewhere that manifests in decoding PNGs. I thought it was specific to the SqueakJS VM though:
https://github.com/codefrau/SqueakJS/issues/23
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim "Bother" said Pooh, as the IRS kicked his door in.
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com wrote:
I don't see any problems with this PNG on Windows in an updated trunk image.
OK, that really confuses me.
It looks identical on - raspberry pi 5.3 32bit - raspberry pi 5.3 64bit - raspberry pi 6 trunk 64 bit - iMac intel 6 trunk 64 bit - iMac intel 5.3 64bit - intel ubuntu 6 trunk 64bit - intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in Squeak?
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
Hi, Not sure if it's been altered. Can you send the image again as attachment, so the browser doesn't interfere in the middle?
Best, Karl
On Sun, Feb 26, 2023 at 7:25 PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com wrote:
I don't see any problems with this PNG on Windows in an updated trunk
image.
OK, that really confuses me.
It looks identical on
- raspberry pi 5.3 32bit
- raspberry pi 5.3 64bit
- raspberry pi 6 trunk 64 bit
- iMac intel 6 trunk 64 bit
- iMac intel 5.3 64bit
- intel ubuntu 6 trunk 64bit
- intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in Squeak?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
No issue on Mac ARM either, looks good.
Maybe send it inside a zip.
Vanessa
On Sun, Feb 26, 2023 at 10:37 AM karl ramberg karlramberg@gmail.com wrote:
Hi, Not sure if it's been altered. Can you send the image again as attachment, so the browser doesn't interfere in the middle?
Best, Karl
On Sun, Feb 26, 2023 at 7:25 PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com wrote:
I don't see any problems with this PNG on Windows in an updated trunk
image.
OK, that really confuses me.
It looks identical on
- raspberry pi 5.3 32bit
- raspberry pi 5.3 64bit
- raspberry pi 6 trunk 64 bit
- iMac intel 6 trunk 64 bit
- iMac intel 5.3 64bit
- intel ubuntu 6 trunk 64bit
- intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in Squeak?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
On 2023-02-26, at 10:43 AM, Vanessa Freudenberg vanessa@codefrau.net wrote:
No issue on Mac ARM either, looks good.
Maybe send it inside a zip.
Good idea
Vanessa
On Sun, Feb 26, 2023 at 10:37 AM karl ramberg karlramberg@gmail.com wrote: Hi, Not sure if it's been altered. Can you send the image again as attachment, so the browser doesn't interfere in the middle?
Best, Karl
On Sun, Feb 26, 2023 at 7:25 PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com wrote:
I don't see any problems with this PNG on Windows in an updated trunk image.
OK, that really confuses me.
It looks identical on
- raspberry pi 5.3 32bit
- raspberry pi 5.3 64bit
- raspberry pi 6 trunk 64 bit
- iMac intel 6 trunk 64 bit
- iMac intel 5.3 64bit
- intel ubuntu 6 trunk 64bit
- intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in Squeak?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful Latin Phrases:- Tam exanimis quam tunica nehru fio. = I am as dead as the nehru jacket.
I see the bug now. The PNG is rasterized as you showed in the first email.
Best, Karl
On Sun, Feb 26, 2023 at 8:02 PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-26, at 10:43 AM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
No issue on Mac ARM either, looks good.
Maybe send it inside a zip.
Good idea
Vanessa
On Sun, Feb 26, 2023 at 10:37 AM karl ramberg karlramberg@gmail.com
wrote:
Hi, Not sure if it's been altered. Can you send the image again as attachment, so the browser doesn't
interfere in the middle?
Best, Karl
On Sun, Feb 26, 2023 at 7:25 PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com
wrote:
I don't see any problems with this PNG on Windows in an updated trunk
image.
OK, that really confuses me.
It looks identical on
- raspberry pi 5.3 32bit
- raspberry pi 5.3 64bit
- raspberry pi 6 trunk 64 bit
- iMac intel 6 trunk 64 bit
- iMac intel 5.3 64bit
- intel ubuntu 6 trunk 64bit
- intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in Squeak?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful Latin Phrases:- Tam exanimis quam tunica nehru fio. = I am as dead as the nehru jacket.
Confirming on 64 bit Intel linux with the zipped file.
I see the bug on Squeak 6 with opensmalltalk-vm and the same with an up to date interpreter VM on trunk level V3 image.
With the same interpreter VM, I do not see the bug on Squeak 3.6, 3.8, 3.9, or 4.5. But with Squeak 4.6 the bug is present.
I get the same results with a Cog VM comparing 4.5 to 4.6.
So it looks like something in the image (not VM or plugins), and it seems to have appeared some time between the Squeak 4.5 and Squeak 4.6 releases.
Dave
On Sun, Feb 26, 2023 at 08:14:06PM +0100, karl ramberg wrote:
I see the bug now. The PNG is rasterized as you showed in the first email.
Best, Karl
On Sun, Feb 26, 2023 at 8:02???PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-26, at 10:43 AM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
No issue on Mac ARM either, looks good.
Maybe send it inside a zip.
Good idea
Vanessa
On Sun, Feb 26, 2023 at 10:37 AM karl ramberg karlramberg@gmail.com
wrote:
Hi, Not sure if it's been altered. Can you send the image again as attachment, so the browser doesn't
interfere in the middle?
Best, Karl
On Sun, Feb 26, 2023 at 7:25???PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com
wrote:
I don't see any problems with this PNG on Windows in an updated trunk
image.
OK, that really confuses me.
It looks identical on
- raspberry pi 5.3 32bit
- raspberry pi 5.3 64bit
- raspberry pi 6 trunk 64 bit
- iMac intel 6 trunk 64 bit
- iMac intel 5.3 64bit
- intel ubuntu 6 trunk 64bit
- intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in Squeak?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful Latin Phrases:- Tam exanimis quam tunica nehru fio. = I am as dead as the nehru jacket.
Narrowing it down some more:
By replaying the update stream in a Squeak 4.5 image, I can see that the problem is introduced in update-eem.279, and that the package change that introduced it is Compression-eem.41.
However, the package has only two method changes, and reverting these methods in a Squeak trunk image does not make the the problem go away, so maybe there are some other related changes later in the update stream.
I don't understand the actual problem, but maybe this will help narrow it down.
Dave
sun, Feb 26, 2023 at 02:52:12PM -0500, David T. Lewis wrote:
Confirming on 64 bit Intel linux with the zipped file.
I see the bug on Squeak 6 with opensmalltalk-vm and the same with an up to date interpreter VM on trunk level V3 image.
With the same interpreter VM, I do not see the bug on Squeak 3.6, 3.8, 3.9, or 4.5. But with Squeak 4.6 the bug is present.
I get the same results with a Cog VM comparing 4.5 to 4.6.
So it looks like something in the image (not VM or plugins), and it seems to have appeared some time between the Squeak 4.5 and Squeak 4.6 releases.
Dave
On Sun, Feb 26, 2023 at 08:14:06PM +0100, karl ramberg wrote:
I see the bug now. The PNG is rasterized as you showed in the first email.
Best, Karl
On Sun, Feb 26, 2023 at 8:02???PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-26, at 10:43 AM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
No issue on Mac ARM either, looks good.
Maybe send it inside a zip.
Good idea
Vanessa
On Sun, Feb 26, 2023 at 10:37 AM karl ramberg karlramberg@gmail.com
wrote:
Hi, Not sure if it's been altered. Can you send the image again as attachment, so the browser doesn't
interfere in the middle?
Best, Karl
On Sun, Feb 26, 2023 at 7:25???PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com
wrote:
I don't see any problems with this PNG on Windows in an updated trunk
image.
OK, that really confuses me.
It looks identical on
- raspberry pi 5.3 32bit
- raspberry pi 5.3 64bit
- raspberry pi 6 trunk 64 bit
- iMac intel 6 trunk 64 bit
- iMac intel 5.3 64bit
- intel ubuntu 6 trunk 64bit
- intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in Squeak?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful Latin Phrases:- Tam exanimis quam tunica nehru fio. = I am as dead as the nehru jacket.
Nice detective work.
I just sat down to take a look at this again... and the power goes off so I have to shut everything down. Oh joy.
On 2023-02-26, at 2:52 PM, David T. Lewis lewis@mail.msen.com wrote:
Narrowing it down some more:
By replaying the update stream in a Squeak 4.5 image, I can see that the problem is introduced in update-eem.279, and that the package change that introduced it is Compression-eem.41.
However, the package has only two method changes, and reverting these methods in a Squeak trunk image does not make the the problem go away, so maybe there are some other related changes later in the update stream.
I don't understand the actual problem, but maybe this will help narrow it down.
Dave
sun, Feb 26, 2023 at 02:52:12PM -0500, David T. Lewis wrote:
Confirming on 64 bit Intel linux with the zipped file.
I see the bug on Squeak 6 with opensmalltalk-vm and the same with an up to date interpreter VM on trunk level V3 image.
With the same interpreter VM, I do not see the bug on Squeak 3.6, 3.8, 3.9, or 4.5. But with Squeak 4.6 the bug is present.
I get the same results with a Cog VM comparing 4.5 to 4.6.
So it looks like something in the image (not VM or plugins), and it seems to have appeared some time between the Squeak 4.5 and Squeak 4.6 releases.
Dave
On Sun, Feb 26, 2023 at 08:14:06PM +0100, karl ramberg wrote:
I see the bug now. The PNG is rasterized as you showed in the first email.
Best, Karl
On Sun, Feb 26, 2023 at 8:02???PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-26, at 10:43 AM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
No issue on Mac ARM either, looks good.
Maybe send it inside a zip.
Good idea
Vanessa
On Sun, Feb 26, 2023 at 10:37 AM karl ramberg karlramberg@gmail.com
wrote:
Hi, Not sure if it's been altered. Can you send the image again as attachment, so the browser doesn't
interfere in the middle?
Best, Karl
On Sun, Feb 26, 2023 at 7:25???PM tim Rowledge tim@rowledge.org wrote:
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com
wrote:
I don't see any problems with this PNG on Windows in an updated trunk
image.
OK, that really confuses me.
It looks identical on
- raspberry pi 5.3 32bit
- raspberry pi 5.3 64bit
- raspberry pi 6 trunk 64 bit
- iMac intel 6 trunk 64 bit
- iMac intel 5.3 64bit
- intel ubuntu 6 trunk 64bit
- intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in Squeak?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful Latin Phrases:- Tam exanimis quam tunica nehru fio. = I am as dead as the nehru jacket.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: CAO: Compare Apples to Oranges
Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5, in 4.6 I see the same problem as in 6.0.
Vanessa
On Sun, Feb 26, 2023 at 2:52 PM David T. Lewis lewis@mail.msen.com wrote:
Narrowing it down some more:
By replaying the update stream in a Squeak 4.5 image, I can see that the problem is introduced in update-eem.279, and that the package change that introduced it is Compression-eem.41.
However, the package has only two method changes, and reverting these methods in a Squeak trunk image does not make the the problem go away, so maybe there are some other related changes later in the update stream.
I don't understand the actual problem, but maybe this will help narrow it down.
Dave
sun, Feb 26, 2023 at 02:52:12PM -0500, David T. Lewis wrote:
Confirming on 64 bit Intel linux with the zipped file.
I see the bug on Squeak 6 with opensmalltalk-vm and the same with an up to date interpreter VM on trunk level V3 image.
With the same interpreter VM, I do not see the bug on Squeak 3.6, 3.8, 3.9, or 4.5. But with Squeak 4.6 the bug is present.
I get the same results with a Cog VM comparing 4.5 to 4.6.
So it looks like something in the image (not VM or plugins), and it seems to have appeared some time between the Squeak 4.5 and Squeak 4.6 releases.
Dave
On Sun, Feb 26, 2023 at 08:14:06PM +0100, karl ramberg wrote:
I see the bug now. The PNG is rasterized as you showed in the first email.
Best, Karl
On Sun, Feb 26, 2023 at 8:02???PM tim Rowledge tim@rowledge.org
wrote:
On 2023-02-26, at 10:43 AM, Vanessa Freudenberg <
vanessa@codefrau.net>
wrote:
No issue on Mac ARM either, looks good.
Maybe send it inside a zip.
Good idea
Vanessa
On Sun, Feb 26, 2023 at 10:37 AM karl ramberg <
karlramberg@gmail.com>
wrote:
Hi, Not sure if it's been altered. Can you send the image again as attachment, so the browser doesn't
interfere in the middle?
Best, Karl
On Sun, Feb 26, 2023 at 7:25???PM tim Rowledge tim@rowledge.org
wrote:
On 2023-02-25, at 10:53 PM, karl ramberg karlramberg@gmail.com
wrote:
I don't see any problems with this PNG on Windows in an updated
trunk
image.
OK, that really confuses me.
It looks identical on
- raspberry pi 5.3 32bit
- raspberry pi 5.3 64bit
- raspberry pi 6 trunk 64 bit
- iMac intel 6 trunk 64 bit
- iMac intel 5.3 64bit
- intel ubuntu 6 trunk 64bit
- intel ubuntu 5.3 64bit
What could make it work on Windows? 64 or 32bit? Windows version?
Is there any chance the file got altered before you loaded it in
Squeak?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RBR: Remove Bits Randomly
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful Latin Phrases:- Tam exanimis quam tunica nehru fio. = I am as
dead
as the nehru jacket.
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg vanessa@codefrau.net wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5, in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo |
tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValue ifTrue: [ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.
Yawn - time for bed.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
Le lun. 27 févr. 2023 à 07:15, tim Rowledge tim@rowledge.org a écrit :
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5,
in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all,
err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in disguise, but on 64 bits, that's 100% So maybe we did not notice the bug before, or maybe it was an optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...) I'll have a look.
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo | tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValue ifTrue:[ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.
Yawn - time for bed.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 07:15, tim Rowledge tim@rowledge.org a écrit :
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5,
in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all,
err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in disguise, but on 64 bits, that's 100% So maybe we did not notice the bug before, or maybe it was an optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...) I'll have a look.
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo | tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValueifTrue: [ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.
Yawn - time for bed.
I changed this method here:
https://source.squeak.org/trunk/Graphics-nice.292.diff
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
Le lun. 27 févr. 2023 à 09:29, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 07:15, tim Rowledge tim@rowledge.org a écrit :
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5,
in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all,
err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in disguise, but on 64 bits, that's 100% So maybe we did not notice the bug before, or maybe it was an optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...) I'll have a look.
Ah OK, since opaque has leading 16rFF byte, normalize was not necessary in 32bits era. Dave introduced the normalize fix later for 64bits:
https://source.squeak.org/trunk/Graphics-dtl.377.diff
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo | tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValueifTrue: [ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.
Yawn - time for bed.
I changed this method here:
https://source.squeak.org/trunk/Graphics-nice.292.diff
In a few words :
a tRNS chunk contains 3 16bits R-G-B pixel values that must be handled as fully transparent But previously, we did not decode those properly (see processTransparencyChunk) !
And we did test for transparency **AFTER** converting the pixel to our internal 8bit channel, which may convert many more pixels to transparent than specified by the transparency chunk...
http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html clearly states that we should tests for transparency first:
Note: when dealing with 16-bit grayscale or truecolor data, it is important to compare both bytes of the sample values to determine whether a pixel is transparent. Although decoders may drop the low-order byte of the samples for display, this must not occur until after the data has been tested for transparency. For example, if the grayscale level 0x0001 is specified to be transparent, it would be incorrect to compare only the high-order byte and decide that 0x0002 is also transparent.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
Ah, found it, obviously, this is usage of Form over rather than Form paint: it cannot work in interlaced... We need to find the right rule that will also work with transparency...
Le lun. 27 févr. 2023 à 11:05, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:29, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 07:15, tim Rowledge tim@rowledge.org a écrit :
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in
4.5, in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all,
err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in disguise, but on 64 bits, that's 100% So maybe we did not notice the bug before, or maybe it was an optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...) I'll have a look.
Ah OK, since opaque has leading 16rFF byte, normalize was not necessary in 32bits era. Dave introduced the normalize fix later for 64bits:
https://source.squeak.org/trunk/Graphics-dtl.377.diff
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo | tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValueifTrue: [ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.
Yawn - time for bed.
I changed this method here:
https://source.squeak.org/trunk/Graphics-nice.292.diff
In a few words :
a tRNS chunk contains 3 16bits R-G-B pixel values that must be handled as fully transparent But previously, we did not decode those properly (see processTransparencyChunk) !
And we did test for transparency **AFTER** converting the pixel to our internal 8bit channel, which may convert many more pixels to transparent than specified by the transparency chunk...
http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html clearly states that we should tests for transparency first:
Note: when dealing with 16-bit grayscale or truecolor data, it is important to compare both bytes of the sample values to determine whether a pixel is transparent. Although decoders may drop the low-order byte of the samples for display, this must not occur until after the data has been tested for transparency. For example, if the grayscale level 0x0001 is specified to be transparent, it would be incorrect to compare only the high-order byte and decide that 0x0002 is also transparent.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
In these methods:
copyPixelsRGB: copyPixelsRGB: at: by: copyPixelsRGBA: copyPixelsRGBA: at: by:
Change to use rule 34 or 44 seems to work with both opaque and transparent PNGs ... tempForm displayOn: form at: 0@y rule: 34. ...
Best, Karl
On Mon, Feb 27, 2023 at 12:10 PM Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:
Ah, found it, obviously, this is usage of Form over rather than Form paint: it cannot work in interlaced... We need to find the right rule that will also work with transparency...
Le lun. 27 févr. 2023 à 11:05, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:29, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 07:15, tim Rowledge tim@rowledge.org a écrit :
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in
4.5, in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all,
err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in disguise, but on 64 bits, that's 100% So maybe we did not notice the bug before, or maybe it was an optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...) I'll have a look.
Ah OK, since opaque has leading 16rFF byte, normalize was not necessary in 32bits era. Dave introduced the normalize fix later for 64bits:
https://source.squeak.org/trunk/Graphics-dtl.377.diff
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo | tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValueifTrue: [ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.
Yawn - time for bed.
I changed this method here:
https://source.squeak.org/trunk/Graphics-nice.292.diff
In a few words :
a tRNS chunk contains 3 16bits R-G-B pixel values that must be handled as fully transparent But previously, we did not decode those properly (see processTransparencyChunk) !
And we did test for transparency **AFTER** converting the pixel to our internal 8bit channel, which may convert many more pixels to transparent than specified by the transparency chunk...
http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html clearly states that we should tests for transparency first:
Note: when dealing with 16-bit grayscale or truecolor data, it is important to compare both bytes of the sample values to determine whether a pixel is transparent. Although decoders may drop the low-order byte of the samples for display, this must not occur until after the data has been tested for transparency. For example, if the grayscale level 0x0001 is specified to be transparent, it would be incorrect to compare only the high-order byte and decide that 0x0002 is also transparent.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
Hi Karl,
Le lun. 27 févr. 2023 à 13:11, karl ramberg karlramberg@gmail.com a écrit :
In these methods:
copyPixelsRGB: copyPixelsRGB: at: by: copyPixelsRGBA: copyPixelsRGBA: at: by:
Change to use rule 34 or 44 seems to work with both opaque and transparent PNGs
Good, but they are not strictly the same, this would deserve some tests...
... tempForm displayOn: form at: 0@y rule: 34. ...
Best, Karl
Note that we can continue using Form over in non interlaced copyPixelsRGB: and copyPixelsRGBA:
I also notice that copyPixelsRGBA:at:by: uses paintAlpha rule (31) which is weird since this rule is using a constant alpha
I think that I tested transparency back in 2014 with http://www.schaik.com/pngsuite/pngsuite_trn_png.html Unfortunately, these contain no example of interlaced mixed with transparency...
Nicolas
On Mon, Feb 27, 2023 at 12:10 PM Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:
Ah, found it, obviously, this is usage of Form over rather than Form paint: it cannot work in interlaced... We need to find the right rule that will also work with transparency...
Le lun. 27 févr. 2023 à 11:05, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:29, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 07:15, tim Rowledge tim@rowledge.org a écrit :
> On 2023-02-26, at 3:21 PM, Vanessa Freudenberg < vanessa@codefrau.net> wrote: > > Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5, in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all,
err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in disguise, but on 64 bits, that's 100% So maybe we did not notice the bug before, or maybe it was an optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...) I'll have a look.
Ah OK, since opaque has leading 16rFF byte, normalize was not necessary in 32bits era. Dave introduced the normalize fix later for 64bits:
https://source.squeak.org/trunk/Graphics-dtl.377.diff
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo | tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValueifTrue: [ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.
Yawn - time for bed.
I changed this method here:
https://source.squeak.org/trunk/Graphics-nice.292.diff
In a few words :
a tRNS chunk contains 3 16bits R-G-B pixel values that must be handled as fully transparent But previously, we did not decode those properly (see processTransparencyChunk) !
And we did test for transparency **AFTER** converting the pixel to our internal 8bit channel, which may convert many more pixels to transparent than specified by the transparency chunk...
http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html clearly states that we should tests for transparency first:
Note: when dealing with 16-bit grayscale or truecolor data, it is important to compare both bytes of the sample values to determine whether a pixel is transparent. Although decoders may drop the low-order byte of the samples for display, this must not occur until after the data has been tested for transparency. For example, if the grayscale level 0x0001 is specified to be transparent, it would be incorrect to compare only the high-order byte and decide that 0x0002 is also transparent.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
Those updates seem to work for all my examples, so thanks.
In order to see about testing thoroughly I applied some advanced expert level google-fu and discovered http://www.schaik.com/pngsuite/ which claims to have test images for every case. There is a convenient downloadable archive for offline testing; quickly scanning it suggests we are doing pretty well though quite a few images seem to be a bit darker in Squeak than in Safari. There are even some crafted bad format images that could help us catch errors more helpfully.
On 2023-02-27, at 6:12 AM, Nicolas Cellier nicolas.cellier.aka.nice@gmail.com wrote:
Hi Karl,
Le lun. 27 févr. 2023 à 13:11, karl ramberg karlramberg@gmail.com a écrit : In these methods:
copyPixelsRGB: copyPixelsRGB: at: by: copyPixelsRGBA: copyPixelsRGBA: at: by:
Change to use rule 34 or 44 seems to work with both opaque and transparent PNGs
Good, but they are not strictly the same, this would deserve some tests... ... tempForm displayOn: form at: 0@y rule: 34. ...
Best, Karl
Note that we can continue using Form over in non interlaced copyPixelsRGB: and copyPixelsRGBA:
I also notice that copyPixelsRGBA:at:by: uses paintAlpha rule (31) which is weird since this rule is using a constant alpha
I think that I tested transparency back in 2014 with http://www.schaik.com/pngsuite/pngsuite_trn_png.html Unfortunately, these contain no example of interlaced mixed with transparency...
Nicolas
On Mon, Feb 27, 2023 at 12:10 PM Nicolas Cellier nicolas.cellier.aka.nice@gmail.com wrote: Ah, found it, obviously, this is usage of Form over rather than Form paint: it cannot work in interlaced... We need to find the right rule that will also work with transparency...
Le lun. 27 févr. 2023 à 11:05, Nicolas Cellier nicolas.cellier.aka.nice@gmail.com a écrit :
Le lun. 27 févr. 2023 à 09:29, Nicolas Cellier nicolas.cellier.aka.nice@gmail.com a écrit :
Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier nicolas.cellier.aka.nice@gmail.com a écrit :
Le lun. 27 févr. 2023 à 07:15, tim Rowledge tim@rowledge.org a écrit :
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg vanessa@codefrau.net wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5, in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all, err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in disguise, but on 64 bits, that's 100% So maybe we did not notice the bug before, or maybe it was an optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...) I'll have a look.
Ah OK, since opaque has leading 16rFF byte, normalize was not necessary in 32bits era. Dave introduced the normalize fix later for 64bits:
https://source.squeak.org/trunk/Graphics-dtl.377.diff
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo | tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValue ifTrue: [ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.Yawn - time for bed.
I changed this method here:
https://source.squeak.org/trunk/Graphics-nice.292.diff
In a few words : a tRNS chunk contains 3 16bits R-G-B pixel values that must be handled as fully transparent But previously, we did not decode those properly (see processTransparencyChunk) !
And we did test for transparency **AFTER** converting the pixel to our internal 8bit channel, which may convert many more pixels to transparent than specified by the transparency chunk...
http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html clearly states that we should tests for transparency first:
Note: when dealing with 16-bit grayscale or truecolor data, it is important to compare both bytes of the sample values to determine whether a pixel is transparent. Although decoders may drop the low-order byte of the samples for display, this must not occur until after the data has been tested for transparency. For example, if the grayscale level 0x0001 is specified to be transparent, it would be incorrect to compare only the high-order byte and decide that 0x0002 is also transparent.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim The enema of my enemy is my friend
Yes, I used them back in 2014, just not all of them. Since I was testing transparency I only used the transparency set.
Now, If I test interlaced vs non interlaced like this:
form8i := PNGReadWriter formFromFileNamed: 'c:\users\cellier\downloads\PngSuite-2017jul19\basi6a08.png'. form16i := PNGReadWriter formFromFileNamed: 'c:\users\cellier\downloads\PngSuite-2017jul19\basi6a16.png'.
form8n := PNGReadWriter formFromFileNamed: 'c:\users\cellier\downloads\PngSuite-2017jul19\basn6a08.png'. form16n := PNGReadWriter formFromFileNamed: 'c:\users\cellier\downloads\PngSuite-2017jul19\basn6a16.png'.
{ form8i bits asByteArray = form8n bits asByteArray. form16i bits asByteArray = form16n bits asByteArray. }
It fails with my recent changes (answer false,false), but works with rule 31 (alphaPaint)
alphaPaint rule is (BitBltSimulation alphaPaintConst: sourceWord with: destinationWord) with sourceAlpha = 255 (the default value used in copyBits, ^ self copyBitsTranslucent: 255)
It does indeed the right thing:
unAlpha := 255 - sourceAlpha. "that is zero" result := destinationWord. "case 32bpp blends include alpha" paintMode & (sourceWord = 0) "painting a transparent pixel" ifFalse:[
blendRB := ((sourceWord bitAnd: 16rFF00FF) * sourceAlpha) + ((destinationWord bitAnd: 16rFF00FF) * unAlpha) + 16r800080. "blend red and blue"
blendAG := ((sourceWord>> 8 bitAnd: 16rFF00FF) * sourceAlpha) + ((destinationWord>>8 bitAnd: 16rFF00FF) * unAlpha) + 16r800080. "blend alpha and green"
blendRB := (blendRB >> 8 bitAnd: 16rFF00FF) + blendRB >> 8 bitAnd: 16rFF00FF. "divide by 255" blendAG := (blendAG >> 8 bitAnd: 16rFF00FF) + blendAG >> 8 bitAnd: 16rFF00FF. result := blendRB bitOr: blendAG<<8.
The important things are: - when sourceWord = 0 (which is the case of non set bits in interlaced mode), destinationWord is preserved - this also blend source and dest alpha channels, unlike I thought I remembered...
So, I will have to revert to paintAlpha and use that in cheap transparency case too. Sorry for the hiccups.
Le lun. 27 févr. 2023 à 19:54, tim Rowledge tim@rowledge.org a écrit :
Those updates seem to work for all my examples, so thanks.
In order to see about testing thoroughly I applied some advanced expert level google-fu and discovered http://www.schaik.com/pngsuite/ which claims to have test images for every case. There is a convenient downloadable archive for offline testing; quickly scanning it suggests we are doing pretty well though quite a few images seem to be a bit darker in Squeak than in Safari. There are even some crafted bad format images that could help us catch errors more helpfully.
On 2023-02-27, at 6:12 AM, Nicolas Cellier <
nicolas.cellier.aka.nice@gmail.com> wrote:
Hi Karl,
Le lun. 27 févr. 2023 à 13:11, karl ramberg karlramberg@gmail.com a
écrit :
In these methods:
copyPixelsRGB: copyPixelsRGB: at: by: copyPixelsRGBA: copyPixelsRGBA: at: by:
Change to use rule 34 or 44 seems to work with both opaque and
transparent PNGs
Good, but they are not strictly the same, this would deserve some
tests...
... tempForm displayOn: form at: 0@y rule: 34. ...
Best, Karl
Note that we can continue using Form over in non interlaced
copyPixelsRGB: and copyPixelsRGBA:
I also notice that copyPixelsRGBA:at:by: uses paintAlpha rule (31) which
is weird
since this rule is using a constant alpha
I think that I tested transparency back in 2014 with
http://www.schaik.com/pngsuite/pngsuite_trn_png.html
Unfortunately, these contain no example of interlaced mixed with
transparency...
Nicolas
On Mon, Feb 27, 2023 at 12:10 PM Nicolas Cellier <
nicolas.cellier.aka.nice@gmail.com> wrote:
Ah, found it, obviously, this is usage of Form over rather than Form
paint: it cannot work in interlaced...
We need to find the right rule that will also work with transparency...
Le lun. 27 févr. 2023 à 11:05, Nicolas Cellier <
nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:29, Nicolas Cellier <
nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier <
nicolas.cellier.aka.nice@gmail.com> a écrit :
Le lun. 27 févr. 2023 à 07:15, tim Rowledge tim@rowledge.org a écrit :
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg vanessa@codefrau.net
wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5,
in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview
in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type
6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes
this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all, err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in
disguise, but on 64 bits, that's 100%
So maybe we did not notice the bug before, or maybe it was an
optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...)
I'll have a look.
Ah OK, since opaque has leading 16rFF byte, normalize was not necessary
in 32bits era.
Dave introduced the normalize fix later for 64bits:
https://source.squeak.org/trunk/Graphics-dtl.377.diff
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo | tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValueifTrue: [
tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.Yawn - time for bed.
I changed this method here:
https://source.squeak.org/trunk/Graphics-nice.292.diff
In a few words : a tRNS chunk contains 3 16bits R-G-B pixel values that must be handled
as fully transparent
But previously, we did not decode those properly (see
processTransparencyChunk) !
And we did test for transparency **AFTER** converting the pixel to our
internal 8bit channel,
which may convert many more pixels to transparent than specified by the
transparency chunk...
http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html clearly states
that we should tests for transparency first:
Note: when dealing with 16-bit grayscale or truecolor data, it is
important to compare both bytes of the sample values to determine whether a pixel is transparent. Although decoders may drop the low-order byte of the samples for display, this must not occur until after the data has been tested for transparency. For example, if the grayscale level 0x0001 is specified to be transparent, it would be incorrect to compare only the high-order byte and decide that 0x0002 is also transparent.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Time to start the War on Errorism before stupidity finally gets us.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim The enema of my enemy is my friend
Hi Nicolas --
Thanks for looking into this! Is it worth porting back to Squeak 6.0? Maybe even 5.3?
Best, Marcel Am 27.02.2023 23:00:37 schrieb Nicolas Cellier nicolas.cellier.aka.nice@gmail.com: Yes, I used them back in 2014, just not all of them.
Since I was testing transparency I only used the transparency set.
Now, If I test interlaced vs non interlaced like this:
form8i := PNGReadWriter formFromFileNamed: 'c:\users\cellier\downloads\PngSuite-2017jul19\basi6a08.png'. form16i := PNGReadWriter formFromFileNamed: 'c:\users\cellier\downloads\PngSuite-2017jul19\basi6a16.png'.
form8n := PNGReadWriter formFromFileNamed: 'c:\users\cellier\downloads\PngSuite-2017jul19\basn6a08.png'. form16n := PNGReadWriter formFromFileNamed: 'c:\users\cellier\downloads\PngSuite-2017jul19\basn6a16.png'.
{ form8i bits asByteArray = form8n bits asByteArray. form16i bits asByteArray = form16n bits asByteArray. }
It fails with my recent changes (answer false,false), but works with rule 31 (alphaPaint)
alphaPaint rule is (BitBltSimulation alphaPaintConst: sourceWord with: destinationWord) with sourceAlpha = 255 (the default value used in copyBits, ^ self copyBitsTranslucent: 255)
It does indeed the right thing:
unAlpha := 255 - sourceAlpha. "that is zero" result := destinationWord.
"case 32bpp blends include alpha" paintMode & (sourceWord = 0) "painting a transparent pixel" ifFalse:[
blendRB := ((sourceWord bitAnd: 16rFF00FF) * sourceAlpha) + ((destinationWord bitAnd: 16rFF00FF) * unAlpha) + 16r800080. "blend red and blue"
blendAG := ((sourceWord>> 8 bitAnd: 16rFF00FF) * sourceAlpha) + ((destinationWord>>8 bitAnd: 16rFF00FF) * unAlpha) + 16r800080. "blend alpha and green"
blendRB := (blendRB >> 8 bitAnd: 16rFF00FF) + blendRB >> 8 bitAnd: 16rFF00FF. "divide by 255" blendAG := (blendAG >> 8 bitAnd: 16rFF00FF) + blendAG >> 8 bitAnd: 16rFF00FF. result := blendRB bitOr: blendAG<<8.
The important things are: - when sourceWord = 0 (which is the case of non set bits in interlaced mode), destinationWord is preserved - this also blend source and dest alpha channels, unlike I thought I remembered...
So, I will have to revert to paintAlpha and use that in cheap transparency case too. Sorry for the hiccups.
Le lun. 27 févr. 2023 à 19:54, tim Rowledge <tim@rowledge.org [mailto:tim@rowledge.org]> a écrit :
Those updates seem to work for all my examples, so thanks.
In order to see about testing thoroughly I applied some advanced expert level google-fu and discovered http://www.schaik.com/pngsuite/ [http://www.schaik.com/pngsuite/] which claims to have test images for every case. There is a convenient downloadable archive for offline testing; quickly scanning it suggests we are doing pretty well though quite a few images seem to be a bit darker in Squeak than in Safari. There are even some crafted bad format images that could help us catch errors more helpfully.
On 2023-02-27, at 6:12 AM, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com [mailto:nicolas.cellier.aka.nice@gmail.com]> wrote:
Hi Karl,
Le lun. 27 févr. 2023 à 13:11, karl ramberg <karlramberg@gmail.com [mailto:karlramberg@gmail.com]> a écrit : In these methods:
copyPixelsRGB: copyPixelsRGB: at: by: copyPixelsRGBA: copyPixelsRGBA: at: by:
Change to use rule 34 or 44 seems to work with both opaque and transparent PNGs
Good, but they are not strictly the same, this would deserve some tests... ... tempForm displayOn: form at: 0@y rule: 34. ...
Best, Karl
Note that we can continue using Form over in non interlaced copyPixelsRGB: and copyPixelsRGBA:
I also notice that copyPixelsRGBA:at:by: uses paintAlpha rule (31) which is weird since this rule is using a constant alpha
I think that I tested transparency back in 2014 with http://www.schaik.com/pngsuite/pngsuite_trn_png.html [http://www.schaik.com/pngsuite/pngsuite_trn_png.html] Unfortunately, these contain no example of interlaced mixed with transparency...
Nicolas On Mon, Feb 27, 2023 at 12:10 PM Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com [mailto:nicolas.cellier.aka.nice@gmail.com]> wrote: Ah, found it, obviously, this is usage of Form over rather than Form paint: it cannot work in interlaced... We need to find the right rule that will also work with transparency...
Le lun. 27 févr. 2023 à 11:05, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com [mailto:nicolas.cellier.aka.nice@gmail.com]> a écrit :
Le lun. 27 févr. 2023 à 09:29, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com [mailto:nicolas.cellier.aka.nice@gmail.com]> a écrit :
Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com [mailto:nicolas.cellier.aka.nice@gmail.com]> a écrit :
Le lun. 27 févr. 2023 à 07:15, tim Rowledge <tim@rowledge.org [mailto:tim@rowledge.org]> a écrit :
On 2023-02-26, at 3:21 PM, Vanessa Freudenberg <vanessa@codefrau.net [mailto:vanessa@codefrau.net]> wrote:
Can confirm that it's not a VM bug – in SqueakJS it works fine in 4.5, in 4.6 I see the same problem as in 6.0.
And in this evenings episode of the Hardy Drew series...
I thought I'd try saving the png fro ma non-squeak png reader (Preview in this case) to see what happens. Probably to no one's surprise the new version loads perfectly in Squeak. A *possibly* relevant factoid I noticed a moment ago during comparison debugging is that the problematic version is tagged as having 24 bits per pixel vs 32 for the good one.
Dang, no, the standard explains that. Color type 2 is indeed 24bpp, type 6 is 32bpp. Then again, it makes the difference between using #copyPixelsRGB: and #copyPixelsRGBA:
Right; it appears that #copyPixelsRGB: was modified in a way that causes this problem. The ' nice 5/10/2014 15:07' version made some fairly large changes to the prior 'nk 7/27/2004 17:18' implementation. A version with a couple of 'pixel normalize' replacing 'pixel' appears to work for all the examples I have to hand. The question remaining of course is what the change Nicolas made got wrong, because if we're honest, he rarely makes mistakes.
Hi Tim,hi all, err no, I make plenty of mistakes, as the average human does. It's just that I intercept most of them before they go to the trunk. Using (LargePositiveInteger new: ) without normalization is suspicious. On 32 bits squeak, it has 1 chance out of 4 to be a SmallInteger in disguise, but on 64 bits, that's 100% So maybe we did not notice the bug before, or maybe it was an optimization at Integer side assuming some invariants (for example LargePositiveInteger isZero answer false or something...) I'll have a look.
Ah OK, since opaque has leading 16rFF byte, normalize was not necessary in 32bits era. Dave introduced the normalize fix later for 64bits:
https://source.squeak.org/trunk/Graphics-dtl.377.diff [https://source.squeak.org/trunk/Graphics-dtl.377.diff]
My hack working version -
copyPixelsRGB: y at: startX by: incX "Handle interlaced RGB color mode (colorType = 2)"
| i pixel tempForm tempBits xx loopsToDo |
tempForm := Form extent: width@1 depth: 32. tempBits := tempForm bits. pixel := LargePositiveInteger new: 4. pixel at: 4 put: 16rFF. loopsToDo := width - startX + incX - 1 // incX. bitsPerChannel = 8 ifTrue: [ i := (startX // incX * 3) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+1); at: 1 put: (thisScanline at: i+2). tempBits at: xx put: pixel normalize. i := i + 3. xx := xx + incX. ] ] ifFalse: [ i := (startX // incX * 6) + 1. xx := startX+1. 1 to: loopsToDo do: [ :j | pixel at: 3 put: (thisScanline at: i); at: 2 put: (thisScanline at: i+2); at: 1 put: (thisScanline at: i+4). tempBits at: xx put: pixel normalize. i := i + 6. xx := xx + incX. ]. ]. transparentPixelValue ifNotNil: [ startX to: width-1 by: incX do: [ :x | (tempBits at: x+1) = transparentPixelValue ifTrue: [ tempBits at: x+1 put: 0. ]. ]. ]. tempForm displayOn: form at: 0@y rule: Form paint.
Yawn - time for bed.
I changed this method here:
https://source.squeak.org/trunk/Graphics-nice.292.diff [https://source.squeak.org/trunk/Graphics-nice.292.diff]
In a few words : a tRNS chunk contains 3 16bits R-G-B pixel values that must be handled as fully transparent But previously, we did not decode those properly (see processTransparencyChunk) !
And we did test for transparency **AFTER** converting the pixel to our internal 8bit channel, which may convert many more pixels to transparent than specified by the transparency chunk...
http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html [http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html] clearly states that we should tests for transparency first:
Note: when dealing with 16-bit grayscale or truecolor data, it is important to compare both bytes of the sample values to determine whether a pixel is transparent. Although decoders may drop the low-order byte of the samples for display, this must not occur until after the data has been tested for transparency. For example, if the grayscale level 0x0001 is specified to be transparent, it would be incorrect to compare only the high-order byte and decide that 0x0002 is also transparent.
tim
tim Rowledge; tim@rowledge.org [mailto:tim@rowledge.org]; http://www.rowledge.org/tim [http://www.rowledge.org/tim] Time to start the War on Errorism before stupidity finally gets us.
tim -- tim Rowledge; tim@rowledge.org [mailto:tim@rowledge.org]; http://www.rowledge.org/tim [http://www.rowledge.org/tim] The enema of my enemy is my friend
On 2023-03-06, at 8:36 AM, Marcel Taeumel via Squeak-dev squeak-dev@lists.squeakfoundation.org wrote:
Hi Nicolas --
Thanks for looking into this! Is it worth porting back to Squeak 6.0? Maybe even 5.3?
I can aver that it does the job very nicely in a 5.3 based image
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: LA: Lockout Access
Le lun. 6 mars 2023 à 18:48, tim Rowledge tim@rowledge.org a écrit :
On 2023-03-06, at 8:36 AM, Marcel Taeumel via Squeak-dev <
squeak-dev@lists.squeakfoundation.org> wrote:
Hi Nicolas --
Thanks for looking into this! Is it worth porting back to Squeak 6.0?
Maybe even 5.3?
I broke it after all, but my POV is that the resolution was collective, a
very nice example of cooperation!
I can aver that it does the job very nicely in a 5.3 based image
Then maybe backport it, I let you judge...
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: LA: Lockout Access
Le lun. 6 mars 2023 à 18:48, tim Rowledge tim@rowledge.org a écrit :
On 2023-03-06, at 8:36 AM, Marcel Taeumel via Squeak-dev <
squeak-dev@lists.squeakfoundation.org> wrote:
Hi Nicolas --
Thanks for looking into this! Is it worth porting back to Squeak 6.0?
Maybe even 5.3?
I broke it after all, but my POV is that the resolution was collective,
a very nice example of cooperation!
I would like to have this fix for my image organizer based on 5.3, but this thread is so far over my head, I couldn't even identify where the problem is, or what the solution is, even after combing through every message. Did I miss it?
I can aver that it does the job very nicely in a 5.3 based image
Then maybe backport it, I let you judge...
+1000, 5.3 is still used, it's worth fixing for a multimedia system, IMO. I would be grateful for a backport. Or, if someone would at least post the changeset fix clearly here, it's possible others would like to have it fixed, too.
Regards, Chris
On 2023-03-23, at 5:05 PM, Chris Muller asqueaker@gmail.com wrote:
I would like to have this fix for my image organizer based on 5.3, but this thread is so far over my head, I couldn't even identify where the problem is, or what the solution is, even after combing through every message. Did I miss it?
The two methods are
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: CAO: Compare Apples to Oranges
squeak-dev@lists.squeakfoundation.org
-
Chris Muller -
David T. Lewis -
karl ramberg -
Marcel Taeumel -
Nicolas Cellier -
tim Rowledge -
Vanessa Freudenberg