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