[squeak-dev] PNGReadWriter buglettim Rowledge tim at rowledge.orgMon Feb 27 18:53:53 UTC 2023
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 at gmail.com> wrote: > > Hi Karl, > > Le lun. 27 févr. 2023 à 13:11, karl ramberg <karlramberg at 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 at 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 at 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 at gmail.com> a écrit : > > > Le lun. 27 févr. 2023 à 09:29, Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com> a écrit : > > > Le lun. 27 févr. 2023 à 09:19, Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com> a écrit : > > > Le lun. 27 févr. 2023 à 07:15, tim Rowledge <tim at rowledge.org> a écrit : > > > > On 2023-02-26, at 3:21 PM, Vanessa Freudenberg <vanessa at 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 at 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 at 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 at rowledge.org; http://www.rowledge.org/tim > Time to start the War on Errorism before stupidity finally gets us. > > > > > tim -- tim Rowledge; tim at rowledge.org; http://www.rowledge.org/tim The enema of my enemy is my friend
More information about the Squeak-dev mailing list |