[squeak-dev] Color from pixel value bug

Nicolas Cellier nicolas.cellier.aka.nice at gmail.com
Mon Oct 3 15:45:45 UTC 2011


Hi Juan & Steph,
I don't remember I messed into this, but indeed, here are my changes:

Nicolas Cellier uploaded a new version of Graphics to project The Trunk:
http://source.squeak.org/trunk/Graphics-nice.161.mcz

==================== Summary ====================

Name: Graphics-nice.161
Author: nice
Time: 14 December 2010, 3:03:16.064 pm
UUID: aa1e48e5-5032-b14d-bc90-84db641c7257
Ancestors: Graphics-ul.160

Fix #testPngDecodingColors32.

This was because (LargePositiveInteger new: 4) ~= 0 in COG VM.
(There is no optimization in primitives for this special case).
Anyway, it is better to always normalize a LargePositiveInteger
created this way.

=============== Diff against Graphics-ul.160 ===============

Item was changed:
 ----- Method: Color>>pixelValueForDepth: (in category 'conversions') -----
 pixelValueForDepth: d
       "Returns an integer representing the bits that appear in a
single pixel of this color in a Form of the given depth. The depth
must be one of 1, 2, 4, 8, 16, or 32. Contrast with pixelWordForDepth:
and bitPatternForDepth:, which return either a 32-bit word packed with
the given pixel value or a multiple-word Bitmap containing a pattern.
The inverse is the class message colorFromPixelValue:depth:"
       "Details: For depths of 8 or less, the result is a colorMap
index. For depths of 16 and 32, it is a direct color value with 5 or 8
bits per color component."
       "Transparency: The pixel value zero is reserved for
transparent. For depths greater than 8, black maps to the darkest
possible blue."

       | rgbBlack val |
       d = 8 ifTrue: [^ self closestPixelValue8].  "common case"
       d < 8 ifTrue: [
               d = 4 ifTrue: [^ self closestPixelValue4].
               d = 2 ifTrue: [^ self closestPixelValue2].
               d = 1 ifTrue: [^ self closestPixelValue1]].

       rgbBlack := 1.  "closest black that is not transparent in RGB"

       d = 16 ifTrue: [
               "five bits per component; top bits ignored"
               val := (((rgb bitShift: -15) bitAnd: 16r7C00) bitOr:
                        ((rgb bitShift: -10) bitAnd: 16r03E0)) bitOr:
                        ((rgb bitShift: -5) bitAnd: 16r001F).
               ^ val = 0 ifTrue: [rgbBlack] ifFalse: [val]].

       d = 32 ifTrue: [
               "eight bits per component; top 8 bits set to all ones
(opaque alpha)"
               val := LargePositiveInteger new: 4.
               val at: 3 put: ((rgb bitShift: -22) bitAnd: 16rFF).
               val at: 2 put: ((rgb bitShift: -12) bitAnd: 16rFF).
               val at: 1 put: ((rgb bitShift: -2) bitAnd: 16rFF).
+               val normalize = 0 ifTrue: [val at: 1 put: 1].
"closest non-transparent black"
-               val = 0 ifTrue: [val at: 1 put: 1].  "closest
non-transparent black"
               val at: 4 put: 16rFF.  "opaque alpha"
+               ^ val normalize].
-               ^ val].

       d = 12 ifTrue: [  "for indexing a color map with 4 bits per
color component"
               val := (((rgb bitShift: -18) bitAnd: 16r0F00) bitOr:
                        ((rgb bitShift: -12) bitAnd: 16r00F0)) bitOr:
                        ((rgb bitShift: -6) bitAnd: 16r000F).
               ^ val = 0 ifTrue: [rgbBlack] ifFalse: [val]].

       d = 9 ifTrue: [  "for indexing a color map with 3 bits per
color component"
               val := (((rgb bitShift: -21) bitAnd: 16r01C0) bitOr:
                        ((rgb bitShift: -14) bitAnd: 16r0038)) bitOr:
                        ((rgb bitShift: -7) bitAnd: 16r0007).
               ^ val = 0 ifTrue: [rgbBlack] ifFalse: [val]].

       self error: 'unknown pixel depth: ', d printString
 !


So you see, I just corrected some side effect (normalize), but not the
kernel of the algorithm.
If you ask me, I would say:
- are there several possible representations for some colors ? (like
100% transparency with any hue)
 in which case comparison must occur after a canonical normalization step
- are there any bug left here and there in the system ?
 that would not amaze me.I'd rather trust Juan's solutions because his
reflexions on the subject are far more advanced than mine, but every
thing should be documented with test cases.

Nicolas

2011/10/3 Juan Vuletich <juan at jvuletich.org>:
> Stéphane Rollandin wrote:
>>
>> Hello,
>>
>> Working in Squeak 4.1, I stumbled upon a PNG image saving bug, which I
>> traced down to the fact that
>>
>> ((Color black pixelWordForDepth: 32) asColorOfDepth: 32)
>>  = Color black
>>
>> is false.
>>
>> I found something looking like a fix, which is attached.
>>
>> Then I looked at Squeak 4.3 (update 11636) to see if someone else already
>> fixed that bug, and indeed Nicolas Cellier did, in December 2010, by
>> modifying Color>>pixelValueForDepth:
>>
>> But we still have
>>
>> ((Color black pixelWordForDepth: 32) asColorOfDepth: 32)
>>  = Color black
>>
>> false, because black is being replaced with, as the comment says, the
>> "closest non-transparent black"
>>
>> At this point I guess I missed something, since I have no idea of what
>> this business of "transparent black" is about.
>>
>> Nicolas, could you please tell if my fix is correct ?
>>
>> Best,
>>
>>
>> Stef
>
> Hi Stef,
>
> BitBlt supports Color transparent in depths < 32bpp, where no alpha channel
> is available. To do this, it considers any pixel with rgb = 0 as transparent
> and not black. So, the convention is to use the darkest possible blue
> instead of black.
> See that the following draws nothing.
> | f |
> f := Form extent: 32 at 32 depth: 16.
> f bits atAllPut: 16r00000000.
> Display getCanvas image: f at: 10 at 10.    "This is the method for displaying
> a possibly translucent form in Cuis. Not sure about Squeak."
>
> This "zero means transparent" trick is not needed in 32bpp, because real
> transparent is available (alpha=0), and especially because BitBlt considers
> transparent only pixels with _all_ the bits at zero. So, in 32bpp, pixels
> with r=g=b=0 but alpha>0 are not considered transparent. There have been
> bugs historically with respect to this detail in the image.
> See that this draws a black box
> | f |
> f := Form extent: 32 at 32 depth: 32.
> f bits atAllPut: 16rFF000000.
> Display getCanvas image: f at: 10 at 10    "This is the method for displaying a
> possibly translucent form in Cuis. Not sure about Squeak."
> This shows that in the 32bpp case it is not needed to replace black by
> darkest blue, plain black can be used.
>
> I think your fix is not correct, as Nicolas' code isn't either. It is true
> that both methods must match, but this special fake black is not needed for
> 32bpp.
> After playing a bit with this today, I updated the Cuis code to reflect all
> this. Find it below. Thanks for bringing this up!
>
> Cheers,
> Juan Vuletich
> --------------------------------------------
> colorFromPixelValue: p depth: d
>   "Convert a pixel value for the given display depth into a color."
>   "Details: For depths of 8 or less, the pixel value is simply looked up in
> a table. For greater depths, the color components are extracted and
> converted into a color."
>   "Warning: In BitBlt, a pixel with pixelValue = 0 is transparent.
>   Squeak usually assumes that r=g=b=0 => transparent. But this is false if
> we have alpha (opacity).
>   A color with r=g=b=0 and opacity = 255 is BLACK, not TRANSPARENT.
>   Squeak also answers darkest possible blue when asked for black. Again,
> this is not needed in 32 bits (with alpha).
>   The real rule is that pixelValue=0 means transparent.
>   And that darkest blue must be used instead of black, but only for depths
>>8 and < 32 (no indexed colors, no alpha)
>   This method is updated to reflect that."
>
>   | r g b alpha |
>   d = 8 ifTrue: [^ IndexedColors at: (p bitAnd: 16rFF) + 1].
>   d = 4 ifTrue: [^ IndexedColors at: (p bitAnd: 16r0F) + 1].
>   d = 2 ifTrue: [^ IndexedColors at: (p bitAnd: 16r03) + 1].
>   d = 1 ifTrue: [^ IndexedColors at: (p bitAnd: 16r01) + 1].
>
>   d = 32 ifTrue: [
>       "eight bits per component; 8 bits of alpha"
>       alpha := p bitShift: -24.
>       alpha = 0 ifTrue: [ ^Color transparent ].
>       r := (p bitShift: -16) bitAnd: 16rFF.
>       g := (p bitShift: -8) bitAnd: 16rFF.
>       b := p bitAnd: 16rFF.
>       ^alpha < 255
>           ifTrue: [ (Color r: r g: g b: b range: 255) alpha: alpha asFloat /
> 255.0 ]
>           ifFalse: [ Color r: r g: g b: b range: 255 ]].
>
>   "For the rest of the depths, pixelValue = 0 means transparent, and darkest
> blue is considered to be black."
>   p = 0 ifTrue: [ ^Color transparent ].
>
>   (d = 16) | (d = 15) ifTrue: [
>       "five bits per component"
>       r := (p bitShift: -10) bitAnd: 16r1F.
>       g := (p bitShift: -5) bitAnd: 16r1F.
>       b := p bitAnd: 16r1F.
>       (r = 0 and: [ g = 0 and: [ b = 1]]) ifTrue: [
>           ^Color black ].
>       ^ Color r: r g: g b: b range: 31].
>
>   d = 12 ifTrue: [
>       "four bits per component"
>       r := (p bitShift: -8) bitAnd: 16rF.
>       g := (p bitShift: -4) bitAnd: 16rF.
>       b := p bitAnd: 16rF.
>       (r = 0 and: [ g = 0 and: [ b = 1]]) ifTrue: [
>           ^Color black ].
>       ^ Color r: r g: g b: b range: 15].
>
>   d = 9 ifTrue: [
>       "three bits per component"
>       r := (p bitShift: -6) bitAnd: 16r7.
>       g := (p bitShift: -3) bitAnd: 16r7.
>       b := p bitAnd: 16r7.
>       (r = 0 and: [ g = 0 and: [ b = 1]]) ifTrue: [
>           ^Color black ].
>       ^ Color r: r g: g b: b range: 7].
>
>   self error: 'unknown pixel depth: ', d printString
> ---------------------
> pixelValueForDepth: d
>   "Returns an integer representing the bits that appear in a single pixel of
> this color in a Form of the given depth. The depth must be one of 1, 2, 4,
> 8, 16, or 32. Contrast with pixelWordForDepth: and bitPatternForDepth:,
> which return either a 32-bit word packed with the given pixel value or a
> multiple-word Bitmap containing a pattern. The inverse is the class message
> colorFromPixelValue:depth:"
>   "Details: For depths of 8 or less, the result is a colorMap index. For
> depths of 16 and 32, it is a direct color value with 5 or 8 bits per color
> component."
>   "Transparency: The pixel value zero is reserved for transparent. For
> depths greater than 8 and less than 32 (no Indexed colors, no real alpha),
> black maps to the darkest possible blue.
>   Note that
>       Color transparent class = TranslucentColor
>   this special case is handled in TranslucentColor >> #pixelValueForDepth:
>   "
>
>   | bitBltFakeBlack val |
>   d = 8 ifTrue: [^ self closestPixelValue8].  "common case"
>   d < 8 ifTrue: [
>       d = 4 ifTrue: [^ self closestPixelValue4].
>       d = 2 ifTrue: [^ self closestPixelValue2].
>       d = 1 ifTrue: [^ self closestPixelValue1]].
>
>   d = 32 ifTrue: [
>       "eight bits per component; top 8 bits set to all ones (opaque alpha)"
>       val := LargePositiveInteger new: 4.
>       val at: 3 put: ((rgb bitShift: -22) bitAnd: 16rFF).
>       val at: 2 put: ((rgb bitShift: -12) bitAnd: 16rFF).
>       val at: 1 put: ((rgb bitShift: -2) bitAnd: 16rFF).
>       val at: 4 put: 16rFF.  "opaque alpha"
>       ^ val normalize].
>
>   "For the rest of the depths, pixelValue = 0 means transparent, and darkest
> blue is considered to be black."
>   bitBltFakeBlack := 1.  "closest black that is not transparent in RGB - Not
> for depths <=8 (Indexed) or = 32 (RGBA)"
>
>   d = 16 ifTrue: [
>       "five bits per component; top bits ignored"
>       val := (((rgb bitShift: -15) bitAnd: 16r7C00) bitOr:
>            ((rgb bitShift: -10) bitAnd: 16r03E0)) bitOr:
>            ((rgb bitShift: -5) bitAnd: 16r001F).
>       ^ val = 0 ifTrue: [bitBltFakeBlack] ifFalse: [val]].
>
>   d = 12 ifTrue: [  "for indexing a color map with 4 bits per color
> component"
>       val := (((rgb bitShift: -18) bitAnd: 16r0F00) bitOr:
>            ((rgb bitShift: -12) bitAnd: 16r00F0)) bitOr:
>            ((rgb bitShift: -6) bitAnd: 16r000F).
>       ^ val = 0 ifTrue: [bitBltFakeBlack] ifFalse: [val]].
>
>   d = 9 ifTrue: [  "for indexing a color map with 3 bits per color
> component"
>       val := (((rgb bitShift: -21) bitAnd: 16r01C0) bitOr:
>            ((rgb bitShift: -14) bitAnd: 16r0038)) bitOr:
>            ((rgb bitShift: -7) bitAnd: 16r0007).
>       ^ val = 0 ifTrue: [bitBltFakeBlack] ifFalse: [val]].
>
>   self error: 'unknown pixel depth: ', d printString
>
>
>



More information about the Squeak-dev mailing list