The Trunk: Graphics-pre.404.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

The Trunk: Graphics-pre.404.mcz

commits-2
Patrick Rein uploaded a new version of Graphics to project The Trunk:
http://source.squeak.org/trunk/Graphics-pre.404.mcz

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

Name: Graphics-pre.404
Author: pre
Time: 15 October 2018, 9:57:15.384742 am
UUID: a816199d-b2b6-7247-a8f8-898c605829df
Ancestors: Graphics-pre.403

Removes odd behavior of colorFromPixelValue:depth: which returns transparent instead of 32bit rgb black

=============== Diff against Graphics-pre.403 ===============

Item was changed:
  ----- Method: Color class>>colorFromPixelValue:depth: (in category 'instance creation') -----
  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."
 
  | 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 = 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]) ifTrue: [
  b = 0 ifTrue: [^Color transparent].
  b = 1 ifTrue: [^Color black]].
  ^ Color r: r g: g b: b range: 31].
 
  d = 32 ifTrue: [
  "eight bits per component; 8 bits of alpha"
  r := (p bitShift: -16) bitAnd: 16rFF.
  g := (p bitShift: -8) bitAnd: 16rFF.
  b := p bitAnd: 16rFF.
  alpha := p bitShift: -24.
  alpha = 0 ifTrue: [^Color transparent].
- (r = 0 and: [g = 0 and: [b = 0]])  ifTrue: [^Color transparent].
  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)]].
 
  d = 12 ifTrue: [
  "four bits per component"
  r := (p bitShift: -8) bitAnd: 16rF.
  g := (p bitShift: -4) bitAnd: 16rF.
  b := p bitAnd: 16rF.
  ^ 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.
  ^ Color r: r g: g b: b range: 7].
 
  self error: 'unknown pixel depth: ', d printString
  !


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Graphics-pre.404.mcz

timrowledge


> On 2018-10-15, at 1:57 AM, [hidden email] wrote:
>
> Patrick Rein uploaded a new version of Graphics to project The Trunk:
> http://source.squeak.org/trunk/Graphics-pre.404.mcz
>
> ==================== Summary ====================
>
> Name: Graphics-pre.404
> Author: pre
> Time: 15 October 2018, 9:57:15.384742 am
> UUID: a816199d-b2b6-7247-a8f8-898c605829df
> Ancestors: Graphics-pre.403
>
> Removes odd behavior of colorFromPixelValue:depth: which returns transparent instead of 32bit rgb black

Umm, as best I remember we expect and require 32bit 0.0.0.0 to be transparent. It is an artefact from way back and has caused much confusion; but a lot of code is based on it.

If you've found a way to avoid it and not break all that code - or better yet, I'm misremembering! - then good.


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Advanced design: Upper management doesn't understand it.



Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Graphics-pre.404.mcz

Bert Freudenberg
On Mon, Oct 15, 2018 at 1:17 PM tim Rowledge <[hidden email]> wrote:


> On 2018-10-15, at 1:57 AM, [hidden email] wrote:
>
> Patrick Rein uploaded a new version of Graphics to project The Trunk:
> http://source.squeak.org/trunk/Graphics-pre.404.mcz
>
> ==================== Summary ====================
>
> Name: Graphics-pre.404
> Author: pre
> Time: 15 October 2018, 9:57:15.384742 am
> UUID: a816199d-b2b6-7247-a8f8-898c605829df
> Ancestors: Graphics-pre.403
>
> Removes odd behavior of colorFromPixelValue:depth: which returns transparent instead of 32bit rgb black

Umm, as best I remember we expect and require 32bit 0.0.0.0 to be transparent. It is an artefact from way back and has caused much confusion; but a lot of code is based on it.

0-0-0-0 is transparent because alpha is 0, that is not the issue. But 255-0-0-0 also used to be treated as transparent because that's what you get when converting from a 16 bit form, in which 0-0-0 is treated as transparent by certain bitblt rules, whereas 0-0-1 is black. Cf. fixAlpha.

(Color black pixelValueForDepth: 16) hex
=> 16r0001
(Color black pixelValueForDepth: 32) hex
=> 16rFF000001

(Color transparent pixelValueForDepth: 16) hex
=> 16r0000
(Color transparent pixelValueForDepth: 32) hex
=> 16r00000000

If you've found a way to avoid it and not break all that code - or better yet, I'm misremembering! - then good.
 
We're not doing a lot of 16/32 bpp conversions nowadays so I expect we won't run into too many issues. Someone should check the bitblt rules to see which ones treat the x-0-0-0 value specially.

- Bert -



Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Graphics-pre.404.mcz

patrick.rein
In reply to this post by timrowledge
Hi all,

the change is reverted now but I would like to document our findings and our insights for future changes.

From Berts mail I gathered that the 255-0-0-0 to be transparent is required for 16bit -> 32bit conversion to work. As this has been around for quite some time some code relies on it. At the same time this regularly leads to problems when working with images loaded from external sources. For example, loading a PNG and using collectColor: to recolor it does not work if the PNG includes pure rgb black pixels. For this to work we would have to map pure black to Squeak Form black on loading and reconvert it on storing. This would however destroy information as we can not distinguish between 0-0-1 and 0-0-0 anymore. The other way to go would be to change the 16bit -> 32bit conversion to set alpha explicitly, but I do not know whether that is actually possible efficiently.

Are there other aspects which I am missing? Is it likely for external packages to heavily rely on this functionality?

Bests
Patrick

>
>
>> On 2018-10-15, at 1:57 AM, [hidden email] wrote:
>>
>> Patrick Rein uploaded a new version of Graphics to project The Trunk:
>> http://source.squeak.org/trunk/Graphics-pre.404.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Graphics-pre.404
>> Author: pre
>> Time: 15 October 2018, 9:57:15.384742 am
>> UUID: a816199d-b2b6-7247-a8f8-898c605829df
>> Ancestors: Graphics-pre.403
>>
>> Removes odd behavior of colorFromPixelValue:depth: which returns transparent instead of 32bit rgb black
>
>Umm, as best I remember we expect and require 32bit 0.0.0.0 to be transparent. It is an artefact from way back and has caused much confusion; but a lot of code is based on it.
>
>If you've found a way to avoid it and not break all that code - or better yet, I'm misremembering! - then good.
>
>
>tim
>--
>tim Rowledge; [hidden email]; http://www.rowledge.org/tim
>Advanced design: Upper management doesn't understand it.
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Graphics-pre.404.mcz

Bert Freudenberg
In reply to this post by timrowledge
On Tue, Oct 16, 2018 at 10:29 AM <[hidden email]> wrote:
Hi all,

the change is reverted now but I would like to document our findings and our insights for future changes.

From Berts mail I gathered that the 255-0-0-0 to be transparent is required for 16bit -> 32bit conversion to work. As this has been around for quite some time some code relies on it. At the same time this regularly leads to problems when working with images loaded from external sources. For example, loading a PNG and using collectColor: to recolor it does not work if the PNG includes pure rgb black pixels. For this to work we would have to map pure black to Squeak Form black on loading and reconvert it on storing. This would however destroy information as we can not distinguish between 0-0-1 and 0-0-0 anymore. The other way to go would be to change the 16bit -> 32bit conversion to set alpha explicitly, but I do not know whether that is actually possible efficiently.

Are there other aspects which I am missing? Is it likely for external packages to heavily rely on this functionality?

I didn't say you need to revert this change ;) 

I was only explaining where the odd convention comes from.

Now, early in the dev cycle is a good time to try this. There are certainly other places that need clean up (e.g. isTransparent).

If we can limit the odd color treatment to the 16 bpp case (and only do it when between 16 and 32) I'd be pretty happy.

- Bert -


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Graphics-pre.404.mcz

Eliot Miranda-2


On Tue, Oct 16, 2018 at 3:17 PM Bert Freudenberg <[hidden email]> wrote:
On Tue, Oct 16, 2018 at 10:29 AM <[hidden email]> wrote:
Hi all,

the change is reverted now but I would like to document our findings and our insights for future changes.

From Berts mail I gathered that the 255-0-0-0 to be transparent is required for 16bit -> 32bit conversion to work. As this has been around for quite some time some code relies on it. At the same time this regularly leads to problems when working with images loaded from external sources. For example, loading a PNG and using collectColor: to recolor it does not work if the PNG includes pure rgb black pixels. For this to work we would have to map pure black to Squeak Form black on loading and reconvert it on storing. This would however destroy information as we can not distinguish between 0-0-1 and 0-0-0 anymore. The other way to go would be to change the 16bit -> 32bit conversion to set alpha explicitly, but I do not know whether that is actually possible efficiently.

Are there other aspects which I am missing? Is it likely for external packages to heavily rely on this functionality?

I didn't say you need to revert this change ;) 

I was only explaining where the odd convention comes from.

Now, early in the dev cycle is a good time to try this. There are certainly other places that need clean up (e.g. isTransparent).

If we can limit the odd color treatment to the 16 bpp case (and only do it when between 16 and 32) I'd be pretty happy.

+1.  Why don't we reinstitute the change in trunk and fix as required?

- Bert -

_,,,^..^,,,_
best, Eliot


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Graphics-pre.404.mcz

patrick.rein
Hi everyone,

apologies for the confusions. I was doing this in between other things which obviously did not work out too well :)

I will try this again in a few days but with some more structure. My idea is to first get a better idea of the scope of this change by finding all tests relying on the old behavior and writing some new tests capturing the use cases which should be possible with the new behavior. After that I will remove that line again and we (hopefully) have some more insight into the extent of the effects of that change.

Bests
Patrick

>
>
>On Tue, Oct 16, 2018 at 3:17 PM Bert Freudenberg <[hidden email]> wrote:
>
>On Tue, Oct 16, 2018 at 10:29 AM <[hidden email]> wrote:
>
>Hi all,
>
>the change is reverted now but I would like to document our findings and our insights for future changes.
>
>From Berts mail I gathered that the 255-0-0-0 to be transparent is required for 16bit -> 32bit conversion to work. As this has been around for quite some time some code relies on it. At the same time this regularly leads to problems when working with images loaded from external sources. For example, loading a PNG and using collectColor: to recolor it does not work if the PNG includes pure rgb black pixels. For this to work we would have to map pure black to Squeak Form black on loading and reconvert it on storing. This would however destroy information as we can not distinguish between 0-0-1 and 0-0-0 anymore. The other way to go would be to change the 16bit -> 32bit conversion to set alpha explicitly, but I do not know whether that is actually possible efficiently.
>
>Are there other aspects which I am missing? Is it likely for external packages to heavily rely on this functionality?
>
>
>I didn't say you need to revert this change ;)
>I was only explaining where the odd convention comes from.
>Now, early in the dev cycle is a good time to try this. There are certainly other places that need clean up (e.g. isTransparent).
>If we can limit the odd color treatment to the 16 bpp case (and only do it when between 16 and 32) I'd be pretty happy.
>+1. Why don't we reinstitute the change in trunk and fix as required?
>
>- Bert -
>
>
>_,,,^..^,,,_
>best, Eliot --0000000000003e52660578629c63--

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Graphics-pre.404.mcz

Chris Muller-3
In reply to this post by Eliot Miranda-2
Hi Patrick,

Thanks for your contributions.  If you are using the word, "try" then
that is an indication we should start it out in the Inbox.

Thanks for your cooperation.

Best Regards,
  Chris
On Wed, Oct 17, 2018 at 10:42 AM <[hidden email]> wrote:

>
> Hi everyone,
>
> apologies for the confusions. I was doing this in between other things which obviously did not work out too well :)
>
> I will try this again in a few days but with some more structure. My idea is to first get a better idea of the scope of this change by finding all tests relying on the old behavior and writing some new tests capturing the use cases which should be possible with the new behavior. After that I will remove that line again and we (hopefully) have some more insight into the extent of the effects of that change.
>
> Bests
> Patrick
>
> >
> >
> >On Tue, Oct 16, 2018 at 3:17 PM Bert Freudenberg <[hidden email]> wrote:
> >
> >On Tue, Oct 16, 2018 at 10:29 AM <[hidden email]> wrote:
> >
> >Hi all,
> >
> >the change is reverted now but I would like to document our findings and our insights for future changes.
> >
> >From Berts mail I gathered that the 255-0-0-0 to be transparent is required for 16bit -> 32bit conversion to work. As this has been around for quite some time some code relies on it. At the same time this regularly leads to problems when working with images loaded from external sources. For example, loading a PNG and using collectColor: to recolor it does not work if the PNG includes pure rgb black pixels. For this to work we would have to map pure black to Squeak Form black on loading and reconvert it on storing. This would however destroy information as we can not distinguish between 0-0-1 and 0-0-0 anymore. The other way to go would be to change the 16bit -> 32bit conversion to set alpha explicitly, but I do not know whether that is actually possible efficiently.
> >
> >Are there other aspects which I am missing? Is it likely for external packages to heavily rely on this functionality?
> >
> >
> >I didn't say you need to revert this change ;)
> >I was only explaining where the odd convention comes from.
> >Now, early in the dev cycle is a good time to try this. There are certainly other places that need clean up (e.g. isTransparent).
> >If we can limit the odd color treatment to the 16 bpp case (and only do it when between 16 and 32) I'd be pretty happy.
> >+1. Why don't we reinstitute the change in trunk and fix as required?
> >
> >- Bert -
> >
> >
> >_,,,^..^,,,_
> >best, Eliot --0000000000003e52660578629c63--
>