pixelValueAt: is not robust to hibernation [was: [squeak-dev] The Trunk: MorphicExtras-nice.137.mcz]

David T. Lewis lewis at mail.msen.com
Fri Dec 20 15:36:44 UTC 2013


Moving this to vm-dev for discussion of the BitBlt primitive.

On Fri, Dec 20, 2013 at 02:13:29AM +0100, Nicolas Cellier wrote:
> 2013/12/20 Bert Freudenberg <bert at freudenbergs.de>
> 
> >
> > On 20.12.2013, at 00:55, Nicolas Cellier <
> > nicolas.cellier.aka.nice at gmail.com> wrote:
> >
> > 2013/12/13 Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com>
> >
> >> I feel like it would be better to change pixelValueAt: so as to always
> >> unhibernate, but I did not dare if ever someone had the idea of sending it
> >> in tight loops (one shouldn't, there is BitBlt for that purpose, but who
> >> knows...).
> >>
> >> Maybe we could fail primitivePixelValueAt if receiver is Byte like, and
> >> unhibernate in fallback code.
> >> We would let every other BitBlt primitives accept a ByteArray to allow
> >> BitBlt tricks.
> >>
> >> Thoughts?
> >>
> >>
> > No one raised a comment so far.
> > Isn't it a good idea to fail the primitive?
> >
> >
> > The primitive must fail, yes. That is simply a bug. Before that primitive
> > existed, pixelValueAt: used BitBlt which did the unhibernate.
> >
> > If yes, then it's very simple to implement, just add:
> >
> >     (interpreterProxy isWords: bitmap)    ifFalse:[interpreterProxy
> > primitiveFail].
> >
> >
> > IMHO the primitive should do the same check as BitBlt: verify that the
> > size of the bits is exactly right given width, height, and depth.
> >
> > - Bert -
> >
> 
> I opened http://bugs.squeak.org/view.php?id=7799 just in case.
> 

Thanks for opening the bugs.squeak.org issue. Your patch adds this:

	bitsSize := interpreterProxy byteSizeOf: bitmap.
	((interpreterProxy isWordsOrBytes: bitmap)
		and: [bitsSize = (stride * height * 4 "bytes per word")])
			ifFalse: [^interpreterProxy primitiveFail].

Which translates as:

	bitsSize = interpreterProxy->byteSizeOf(bitmap);
	if (!((interpreterProxy->isWordsOrBytes(bitmap)) && (bitsSize == ((stride * height) * 4)))) {
		interpreterProxy->primitiveFail();
		return null;
	}

This looks like exactly what Bert was suggesting.

I think there are two things we should test:

1) Check if the extra computation has an effect on performance.

2) Check to make sure the logic works on the 64 bit image, to make sure
that the 4 bytes per word does the right thing (I think that it will work,
and I can follow up on this later to be sure).

Dave



More information about the Squeak-dev mailing list