[squeak-dev] PNGReadWriter bugletNicolas Cellier nicolas.cellier.aka.nice at gmail.comMon Feb 27 22:00:09 UTC 2023
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 at 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 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20230227/d000139c/attachment.html>
More information about the Squeak-dev mailing list |