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/26a68f22/attachment.html>