[Vm-dev] Support for reading and writing JPEGs to/from -32, -16, -8 and 8 bit deep forms on V3

Fri May 27 15:52:12 UTC 2016

Elliot,

Thanks for the detailed instructions! They were really helpful. I
managed to refactor the plugin to be more readable. Here's the
corresponding changeset and C code file. which should be placed in
platforms/Cross/plugins/JPEGReadWriter2Plugin

Cheers!
-Laura Perez Cerrato

On 26 May 2016 at 19:09, Eliot Miranda <eliot.miranda at gmail.com> wrote:
> Hi Laura,
>
> On Thu, May 26, 2016 at 7:00 AM, Laura Perez Cerrato
> <lauraperezcerrato at gmail.com> wrote:
>>
>>
>> Elliot, Dave,
>>
>> Thanks for your feedback! I didn't realise the differences between the
>> coding practices of JPEGReadWriter2Plugin and other plugins since I
>> didn't look at the code of the others. I'll try to get the code of
>> JPEGReadWriter2Plugin to bear more resemblance to the rest of the
>> codebase. I've taken a brief look at RePlugin's code and the way the C
>> code should be placed into external files is still a bit confusing to
>> me. Any guidance in this aspect would be greatly appreciated.
>
>
> So the scheme is
> a) the generated C source for the Smalltalk plugin class is generated in
> src/plugins/PluginName/PluginName.c, and it includes a PluginName.h
> b) the PluginName.h lives in
> platforms/Cross/plugins/PluginName/PluginName.h.  It should define the
> functions and types exported by any C sport for the PluginName.c file to
> use.
> c) if the C support code is cross-platform it should live in
> platforms/Cross/plugins/PluginName/sqPluginName.c, which also includes
> PluginName.c.  That's the case here, so refactor the bodies of all plugin
> methods so that the C code tat is in cCode: '...here...' strings gets moved
> to functions in platforms/Cross/plugins/PluginName/sqPluginName.c.  These
> functions can be called anything you like but there's a convention to
> prepend "sq" or "io".  For example in the FilePlugin names like sqFileFlush.
> "io" tends to be for more GUI and OS related functionality.
> d) if the C code is not cross p;at form, or part of it is not cross
> platform, that code goes in
> platforms/platformName/plugins/PluginName/sqPlatPluginName.c, e.g.
> platforms/win32/plugins/UUIDPlugin/sqWin32UUID.c
>
>>
>>
>> As I mentioned earlier, I've been working on handling non-even width
>> 16-bit Forms and non-0 modulo 4 width 8 bit Forms. Here are two
>> changesets with the implementation of that feature.
>
>
> Thanks!!
>
>>
>>
>> Cheers!
>> -Laura Perez Cerrato
>>
>>
>> On 19 May 2016 at 23:05, David T. Lewis <lewis at mail.msen.com> wrote:
>> >
>> > Hi Laura,
>> >
>> > Thanks very much for posting this. I took a quick look at it and will
>> > try to follow up later.
>> >
>> > To clarify one point: Eliot is using the word "horrible" to refer to
>> > some coding practices in the existing JPEGReadWriter2Plugin, and it does
>> > not mean that your patches are "horrible".
>> >
>> >
>> > Eliot,
>> >
>> > Indeed, those giant blocks of #cCode are bad, and somebody (tm) should
>> > do something about it. It may not be obvious from looking at the change
>> > set, but Laura's updates are are patches against that existing code.
>> > So your suggestion is good - either rewrite those portions in proper
>> > Smalltalk slang, or just move them out to C code in the platforms
>> > support.
>> >
>> > I think that this is probably beyond the scope of what Laura is trying
>> > to
>> > do here, although I would encourage anyone with an interest in improving
>> > the plugins to take a look at reworking the #cCode portions of
>> > JPEGReadWriter2Plugin.
>> >
>> > Dave
>> >
>> >
>> >
>> >
>> > On Thu, May 19, 2016 at 04:29:15PM -0700, Eliot Miranda wrote:
>> >>
>> >> Hi Laura,
>> >>
>> >>     this isn't your fault, but looking at the body of the plugin
>> >> methods,
>> >> the code is horrible.  It's essentially 100% C.  The code should really
>> >> be
>> >> written in C files, not in Smalltalk methods, following the pattern of
>> >> separating plugin code into a Smalltalk plugin larger calling platform
>> >> functions written in a C layer.  For example, look at the REPlugin
>> >> which
>> >> does regular expressions.  The source to the machinery is in
>> >> platforms/Cross/plugins/RePlugin.  The plugin methods, e.g.
>> >> REPlugin>>primPCREExecfromto, do marshalling of Smalltalk objects into
>> >> C
>> >> parameters and then invoke function implemented in
>> >> platforms/Cross/plugins/RePlugin, e.g. pcre_exec.  This approach means
>> >> that
>> >> there's onlyas much cCode:inSmalltalk: as necessary.  This is far nicer
>> >> code to read and fix.
>> >>
>> >> Do you have energy to rewrite the JPEG plain code to follow this
>> >> pattern?
>> >> If so, let me encourage you.  It will improve the code hugely.
>> >>
>> >> On Thu, May 19, 2016 at 7:06 AM, Laura Perez Cerrato <
>> >> lauraperezcerrato at gmail.com> wrote:
>> >>
>> >> >
>> >> > Elliot, Dave,
>> >> >
>> >> > Thanks for your answers! Here go both the changesets requested. The
>> >> > JPEGReadWriter2Plugin changeset was generated using a Squeak Spur
>> >> > trunk image, whilst the Graphics changeset was generated using the
>> >> > latest stable version of Squeak non-Spur (4.6-15102). However, It
>> >> > should also work well with a Squeak Spur trunk image, though I
>> >> > haven't
>> >> > tested it's functioning there thoroughly. I haven't yet implemented a
>> >> > graceful way for the new primitive to fail if the new version of the
>> >> > plugin is absent, so keep that in mind.
>> >> >
>> >> > I'm currently working on handling non-even width 16-bit Forms and
>> >> > non-0 modulo 4 width 8 bit Forms, so I'll hopefully send that to you
>> >> > too within this week.
>> >> >
>> >> > -Laura Perez Cerrato
>> >> >
>> >> >
>> >> > On 18 May 2016 at 22:00, David T. Lewis <lewis at mail.msen.com> wrote:
>> >> > >
>> >> > > On Wed, May 18, 2016 at 03:56:11PM -0700, Eliot Miranda wrote:
>> >> > >>
>> >> > >> Hi Laura,
>> >> > >>
>> >> > >> > On May 18, 2016, at 1:04 PM, Laura Perez Cerrato <
>> >> > lauraperezcerrato at gmail.com> wrote:
>> >> > >> >
>> >> > >> >
>> >> > >> > Hello, everyone,
>> >> > >> >
>> >> > >> > During the past few days I've been working on adding support for
>> >> > >> > reading and writing reading and writing JPEGs to/from -32, -16,
>> >> > >> > -8
>> >> > >> > grayscale and 8 grayscale bit deep forms to
>> >> > >> > JPEGReadWriter2Plugin for
>> >> > >> > the Squeak Cog V3 VM. Just chiming in to tell you that the work
>> >> > >> > is
>> >> > >> > done and I'd like to share it with you all. :) I believe this
>> >> > >> > changes
>> >> > >> > should work on Spur VMs too, but haven't tested thoroughly
>> >> > >> > enough to
>> >> > >> > ensure that.
>> >> > >> >
>> >> > >> > How do you handle contributions to the codebase? I assume I'd
>> >> > >> > have to
>> >> > >> > upload the corresponding changeset to the repository but I'm not
>> >> > >> > sure
>> >> > >> > if there's anything I should do before.
>> >> > >> >
>> >> > >> > I have also been working on a few changes to the latest non-Spur
>> >> > >> > version of Squeak in order to take advantage of the changes to
>> >> > >> > the
>> >> > >> > plugin but, since I added a primitive to it and these changes
>> >> > >> > use it,
>> >> > >> > I think it's prudent to ask you too how those contributions are
>> >> > >> > handled. Should I refer to the Squeak-dev mailing list?
>> >> > >>
>> >> > >> This is indeed a problem.  Talking for myself, until VMs have been
>> >> > built that include the code and are generally available one can't
>> >> > really
>> >> > deploy the behaviour to trunk.  But again waiting for the next
>> >> > release
>> >> > feels far too slow.
>> >> > >>
>> >> > >> One approach is to make the behaviour optional, depending on what
>> >> > >> the
>> >> > VM provides, eg checking for primitive failure or a plugin version
>> >> > number,
>> >> > or simply making the code fail gracefully.
>> >> > >>
>> >> > >> Another approach is to provide the functionality in an extension
>> >> > package, and merging that extension into trunk at the next release.
>> >> > >>
>> >> > >> Other suggestions folks?
>> >> > >>
>> >> > >
>> >> > > We should take a look at the image side code too. Laura, perhaps
>> >> > > you can
>> >> > > post a change set for that also?
>> >> > >
>> >> > > Very often a new optional primitive can be handled simply by
>> >> > > providing
>> >> > > some suitable fallback code. That provides a smooth transition,
>> >> > > because
>> >> > > the new feature is immediately available and the system continues
>> >> > > to work
>> >> > > for people who do not yet have the new primitive in their VM.
>> >> > >
>> >> > > Dave
>> >> > >
>> >> >
>> >> >
>> >>
>> >>
>> >> --
>> >> _,,,^..^,,,_
>> >> best, Eliot
>> >
>>
>
>
>
> --
> _,,,^..^,,,_
> best, Eliot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: JPEGReadWriter2Plugin.4.cs
Type: text/x-csharp
Size: 6034 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20160527/577ab8db/JPEGReadWriter2Plugin.4-0001.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sqJPEGReadWriter2Plugin.c
Type: text/x-csrc
Size: 9517 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20160527/577ab8db/sqJPEGReadWriter2Plugin-0001.c