PNG importing appears to be broken ( was Re: [squeak-dev] Who understand bilinear interpolation for reducing image size?)

Bert Freudenberg bert at freudenbergs.de
Thu Dec 11 21:55:53 UTC 2014


On 11.12.2014, at 19:55, tim Rowledge <tim at rowledge.org> wrote:
> 
> On 11-12-2014, at 4:15 AM, Bert Freudenberg <bert at freudenbergs.de> wrote:
>>> 
>>> to use 'combinationRule: Form blend' so that the alpha values are considered.
>> 
>> Don't think so. "Form over" is a simple copy, it copies everything including alpha unmodified. Besides, "form" is empty so there really is no reason to blend.
> 
> And ‘over’ simply copies malformed pixels to the target; those pixels that are 16r00FFFFFF and ought to be 16r0.

IMHO it's not wrong if reading a file results in the exact contents of the file.

> Using ‘blend’ can solve that particular problem, but with the obvious downside that partial alpha pixels can become amusingly wrong. So yeah, it’s not a robust solution.

The Right Way to do blending later is using premultiplied alpha (Porter/Duff, 1984). When you convert 16r00FFFFFF to its pre-multiplied form, it becomes 16r00000000.

The problem is that the Right Way was only implemented in Squeak when some guy who actually understood graphics added bitblt mode 34. Everything else is wrong one way or another.

>> There could potentially be a problem in rgbaDecoderMapForDepth: but it looks okay to me.
> 
> It works very cleverly as a way to handle endian conversions and some depth conversion, but when changing the code to use that the bad-alpha issue was forgotten or otherwise ignored. I’m not totally convinced that the overheads of a fairly complex bitlbt setup for every row of an image  are better than a fairly simple loop to futz each pixel. It’ll depend upon the machine, the virtual machine and the run of data. Given that one then has to do something to each pixel to fix the bad-transparent cases it’s probably not that great.
> 
> Maybe there is a place for a new blt rule as a companion to fixAlpha, that replaces alpha=0 pixels with all-0, perhaps does the mapping of 0 to black->1 etc.

That would be piling on more of the wrong thing.

>>> Obviously a blend is a bit more expensive than an over, but we’re doing a lot of processing to import a png so I doubt it will make a noticeable difference - and it’s likely faster than manually iterating across all the pixels to fix things up.
>> 
>> If you look at alphaBlend:with: you'll see "blend" does not do what you might think it would be doing?
> 
> It doesn’t do exactly what I thought but it doesn’t do what the comment claims either. Specifically the comment claims "The high byte of the result will be 0.” which doesn’t appear to be the case. A simple  workspace fragment derived from the BitBltSimulation shows the alpha value in the result. Which I rather think is what we really want.

The comment is obviously wrong, yes, but otherwise it does do what I expected. In particular, no futzing with FF000001 vs 00000000.

>>> I’m a touch puzzled that I can’t find any code that seems to be doing the ‘black=16rFF000001' fudge we have to do, so maybe that is in need of fixing?
>> 
>> No, the code assumes that if you are actually caring about alpha you know what you're doing. It assumes you will use proper alpha blending to display stuff, in which case fudging isn't needed.
> 
> The black = very very dark blue thing is nothing to do with blending etc. Somewhere we have to decide if an incoming png pixel value is meant to be black and convert that to our pseudo-black. 

No, it really depends on what you are going to do with that pixel. If you have a proper rendering pipeline down the road, FF000000 is a perfectly fine opaque black.

>>> Also I find myself a little suspicious of #copyPixelsGrayAlpha:, copyPixelsGrayAlpha:at:by:, copyPixelsRGBA:at:by:, copyPixelsGray:, copyPixelsGray:at:by: and I’ve probably missed some. Interestingly #copyPixelsRGB: seems to carefully do the right thing to add the alpha channel.
>> 
>> Of course, because unlike its RGBA sister it does not have a real alpha channel to work with. Instead it has to do chroma-keying as a poor-mans replacement.
>> 
>>> Does anyone actually feel they understand this stuff?
>> 
>> It's not rocket science.
> 
> No, it’s *much* more complicated than that.
> 
>> You just need to find the exact point where things go wrong.
> 
> I’m pretty sure that not checking for bad pixel values within these routines is where it goes wrong. I suggest that the fact that an older version of the code fixed the pixels and loaded the files correctly is quite strong supportive evidence.

Two wrongs don't make a right. For an image loader to not give me the pixel values an image was saved with is wrong.

I guess I should take a look at the problem. How can I reproduce?

- Bert -

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4142 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20141211/6a3626b0/smime.bin


More information about the Squeak-dev mailing list