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

[Nicolas Cellier]

2013/12/20 David T. Lewis <lewis at mail.msen.com>

>
> 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.
>

I don't think the difference will be measurable, that's very few ops
overhead...
Even if it was, it must be compared to in-image protection alternative.
So I don't think that this is relevant in this case.


>
> 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
>
>
The 4 bytes per word is also hardcoded in other BitBlt primitive because
that's really what the bit blt expect, 32bit words.
I'm a bit puzzled by 64bits image terminology, is variableWordSubclass:
composed of 32bits words or 64 bits words (like pointers)?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20131220/c07cc550/attachment.htm