[squeak-dev] PNGReadWriter bugletkarl ramberg karlramberg at gmail.comMon Feb 27 12:11:15 UTC 2023
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 at y rule: 34. ... Best, Karl 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. >>>>> >>>>> >>>>> > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20230227/4b03250b/attachment.html>
More information about the Squeak-dev mailing list |