This can be a nitpick of the year, but there is a bug in
InflateStream>>decompressBlock:with: There is a part that reads (toward the end): ------------------------ collection replaceFrom: readLimit+1 to: readLimit + length + 1 with: collection startingAt: readLimit - distance + 1. ------------------------ but here we want to replace "length" bytes in collection, as opposed to length + 1 bytes, so it has to say: ------------------------ collection replaceFrom: readLimit+1 to: readLimit + length with: collection startingAt: readLimit - distance + 1. ------------------------ (So is the non-primitive version in FastInflateStream.) The reason it does not matter is that replaceFrom:to:with:startingAt: does not care about the source and destination being the same and overlap between them. Even when the above bug puts one extra bytes in collection temporarily, the extra byte will be wiped out in the next iteration of the loop surrounding it. However, if you try to port this to a language that does the "right thing" when there is an overlap, it manifests as a bug. -- Yoshiki P.S. It was a "rare" delight to point out a bug in Andreas' code^^; We miss you. |
Thanks Yoshiki,
I'm committing this now On Tue, May 6, 2014 at 2:09 PM, Yoshiki Ohshima <[hidden email]> wrote: This can be a nitpick of the year, but there is a bug in best, Eliot
|
Thank you Eliot!
The one in my previous email was not a real problem, but I noticed that Squeak's PNGReader cannot load certain kind of valid PNG files. For example, when colorType=0 (grayscale), bits per channel = 16, and has tRNS chunk. An example of such is 'tbwn0g16.png' on http://www.schaik.com/pngsuite/pngsuite_trn_png.html. On Tue, May 6, 2014 at 3:02 PM, Eliot Miranda <[hidden email]> wrote: > Thanks Yoshiki, > > I'm committing this now > > > On Tue, May 6, 2014 at 2:09 PM, Yoshiki Ohshima <[hidden email]> > wrote: >> >> This can be a nitpick of the year, but there is a bug in >> InflateStream>>decompressBlock:with: >> >> There is a part that reads (toward the end): >> >> ------------------------ >> collection >> replaceFrom: readLimit+1 >> to: readLimit + length + 1 >> with: collection >> startingAt: readLimit - distance + 1. >> ------------------------ >> >> but here we want to replace "length" bytes in collection, as opposed >> to length + 1 bytes, so it has to say: >> >> ------------------------ >> collection >> replaceFrom: readLimit+1 >> to: readLimit + length >> with: collection >> startingAt: readLimit - distance + 1. >> ------------------------ >> >> (So is the non-primitive version in FastInflateStream.) >> >> The reason it does not matter is that replaceFrom:to:with:startingAt: >> does not care about the source and destination being the same and >> overlap between them. Even when the above bug puts one extra bytes in >> collection temporarily, the extra byte will be wiped out in the next >> iteration of the loop surrounding it. However, if you try to port >> this to a language that does the "right thing" when there is an >> overlap, it manifests as a bug. >> >> -- Yoshiki >> >> P.S. >> It was a "rare" delight to point out a bug in Andreas' code^^; We miss >> you. >> > > > > -- > best, > Eliot -- -- Yoshiki |
Do we support any 16 bit-per-channel gifs?
- Bert - On 09.05.2014, at 06:15, Yoshiki Ohshima <[hidden email]> wrote: > Thank you Eliot! > > The one in my previous email was not a real problem, but I noticed > that Squeak's PNGReader cannot load certain kind of valid PNG files. > For example, when colorType=0 (grayscale), bits per channel = 16, and > has tRNS chunk. An example of such is 'tbwn0g16.png' on > http://www.schaik.com/pngsuite/pngsuite_trn_png.html. > > On Tue, May 6, 2014 at 3:02 PM, Eliot Miranda <[hidden email]> wrote: >> Thanks Yoshiki, >> >> I'm committing this now >> >> >> On Tue, May 6, 2014 at 2:09 PM, Yoshiki Ohshima <[hidden email]> >> wrote: >>> >>> This can be a nitpick of the year, but there is a bug in >>> InflateStream>>decompressBlock:with: >>> >>> There is a part that reads (toward the end): >>> >>> ------------------------ >>> collection >>> replaceFrom: readLimit+1 >>> to: readLimit + length + 1 >>> with: collection >>> startingAt: readLimit - distance + 1. >>> ------------------------ >>> >>> but here we want to replace "length" bytes in collection, as opposed >>> to length + 1 bytes, so it has to say: >>> >>> ------------------------ >>> collection >>> replaceFrom: readLimit+1 >>> to: readLimit + length >>> with: collection >>> startingAt: readLimit - distance + 1. >>> ------------------------ >>> >>> (So is the non-primitive version in FastInflateStream.) >>> >>> The reason it does not matter is that replaceFrom:to:with:startingAt: >>> does not care about the source and destination being the same and >>> overlap between them. Even when the above bug puts one extra bytes in >>> collection temporarily, the extra byte will be wiped out in the next >>> iteration of the loop surrounding it. However, if you try to port >>> this to a language that does the "right thing" when there is an >>> overlap, it manifests as a bug. >>> >>> -- Yoshiki >>> >>> P.S. >>> It was a "rare" delight to point out a bug in Andreas' code^^; We miss >>> you. >>> >> >> >> >> -- >> best, >> Eliot > > > > -- > -- Yoshiki > smime.p7s (5K) Download Attachment |
On Fri, May 9, 2014 at 1:14 AM, Bert Freudenberg <[hidden email]> wrote:
> Do we support any 16 bit-per-channel gifs? Not sure what you mean by "gifs" (I am specifically talking about PNGs), but sure, I think Squeak should be able to read a PNG file, as long as it is a valid one. If it is 16 bits per channel, it should downscale the depth to 8 and create a 32 bpp Forms. This does happen in most of the cases, but when the PNG file specifies the transparent pixel (it is also in 16 bit value), it causes an error. -- -- Yoshiki |
I just committed a fix. The PNGReadWriter currenlty has a hack to support 16 bits per channel gray scale (with a slow bit poker loop), but not with transparency... - transparency is handled with a palette - ColorForm only supports depth <= 8 Since the Form is forced to depth 8, and since we must choose an entry in the palette for transparency, I forced the transparency to be stored at 1st palette entry (value 0) in case of 16 bitsPerChannel. It seems to me that there was another bug even for bitsPerPixel <= 8: the palette is 1-based indexed so transparent color index was one off. It also seems to me that the inversion 255-pixelValue is wrong in the 16 bitsPerChannel bitPoker loop , I did change it. 2014-05-09 19:36 GMT+02:00 Yoshiki Ohshima <[hidden email]>:
|
After further fixes i tested these forms successfully: #( pal_bk pal rgba8_bk rgba8 rgba16_bk rgba16 rgb8_t_bk rgb8_t rgb16_t_bk rgb16_t gray8a_bk gray8a gray16a_bk gray16a gray8b_bk gray8b gray16b_bk gray16b pal_bk_notrns palb ) do: [:e | (ImageReadWriter formFromStream: ('http://entropymine.com/jason/testbed/pngtrans/',e,'.png') asUrl retrieveContents contentStream) asMorph openInWorld]. 2014-05-10 12:36 GMT+02:00 Nicolas Cellier <[hidden email]>:
|
In reply to this post by Nicolas Cellier
There’s a related issue that could do with a fix sometime - the jpeg library we used to make the jepegreadwriteplugin is out of date and Debian (at least) considers that a security risk and thus they mangle the sources to not use those file but link to the ‘standard’ library. Which unfortunately doesn’t actually work within our plugin!
Leaving aside the sheer rudeness of throwing unapproved ‘patches’ into somebody’s code (without ever letting Ian know, for example) and resulting in a broken function, I imagine that if looked into we would be able to tweak our plugin code to actually work with the new jpeg library code. This would be a Good Thing. If anyone knows enough about jpeg library, Debian, blah blah blah to look into this then it might make a nice project that would benefit the Raspberry Pi Scratch using world - several tens of thousands of kids in the UK alone. tim -- tim Rowledge; [hidden email]; http://www.rowledge.org/tim I am the computer your mother warned you about. |
On Sat, May 10, 2014 at 03:00:41PM -0700, tim Rowledge wrote:
> There?s a related issue that could do with a fix sometime - the jpeg library we used to make the jepegreadwriteplugin is out of date and Debian (at least) considers that a security risk and thus they mangle the sources to not use those file but link to the ?standard? library. Which unfortunately doesn?t actually work within our plugin! > > Leaving aside the sheer rudeness of throwing unapproved ?patches? into somebody?s code (without ever letting Ian know, for example) and resulting in a broken function, I imagine that if looked into we would be able to tweak our plugin code to actually work with the new jpeg library code. This would be a Good Thing. If anyone knows enough about jpeg library, Debian, blah blah blah to look into this then it might make a nice project that would benefit the Raspberry Pi Scratch using world - several tens of thousands of kids in the UK alone. > This issue is tracked in Mantis 7539: "Plugins should use platform libraries where possible. Gentoo removed Squeak for this reason." http://bugs.squeak.org/view.php?id=7539 It was originally reported based on Gentoo rejecting Squeak, and it affects Debian and other Linux distributions that (quite reasonably) expect that supported applications should link to current versions of system libraries. If anyone is interested in working to improve this, please do. It would be a great way for someone to contribute a really useful update to the VM. Dave |
2014-05-11 3:48 GMT+02:00 David T. Lewis <[hidden email]>: On Sat, May 10, 2014 at 03:00:41PM -0700, tim Rowledge wrote: A general idea would be to proceed in two steps: -1) upgrade the library in squeak svn repository, so as to fix the API evolutions mismatch -2) provide a way to link to existing library (via cmake/configure in VMMaker and apt/rpm package dependency at distrib level)
Good plan, but what are the starting points? Which library exactly should be used? libjpeg - http://sourceforge.net/projects/libjpeg/ - which is the successor of the library used in squeak is licenced GPL v2, should we taint the whole VM? There is also an opened issue (http://sourceforge.net/p/libjpeg/bugs/5/ - 5 years old!!!) telling it does not compile on win32 (not sure which compiler though, it might be MSVC only), so this might be a hurdle for step 1). So what is the plan in details? |
2014-05-11 9:18 GMT+02:00 Nicolas Cellier <[hidden email]>:
|
In reply to this post by Yoshiki Ohshima-3
On 2014-05-09, at 19:36, Yoshiki Ohshima <[hidden email]> wrote:
> On Fri, May 9, 2014 at 1:14 AM, Bert Freudenberg <[hidden email]> wrote: >> Do we support any 16 bit-per-channel gifs? > > Not sure what you mean by "gifs" Yes, I meant pngs. Slip of mind. > (I am specifically talking about > PNGs), but sure, I think Squeak should be able to read a PNG file, as > long as it is a valid one. If it is 16 bits per channel, it should > downscale the depth to 8 and create a 32 bpp Forms. This does happen > in most of the cases, but when the PNG file specifies the transparent > pixel (it is also in 16 bit value), it causes an error. > -- > -- Yoshiki Ah, okay. I wasn't even aware Squeak reads 16 bpp images at all, because bitblt supports at most 8 bpp. - Bert - smime.p7s (5K) Download Attachment |
In reply to this post by Nicolas Cellier
On Sun, May 11, 2014 at 09:38:06AM +0200, Nicolas Cellier wrote:
> 2014-05-11 9:18 GMT+02:00 Nicolas Cellier < > [hidden email]>: > > > > > 2014-05-11 3:48 GMT+02:00 David T. Lewis <[hidden email]>: > > > > On Sat, May 10, 2014 at 03:00:41PM -0700, tim Rowledge wrote: > >> > There?s a related issue that could do with a fix sometime - the jpeg > >> library we used to make the jepegreadwriteplugin is out of date and Debian > >> (at least) considers that a security risk and thus they mangle the sources > >> to not use those file but link to the ?standard? library. Which > >> unfortunately doesn?t actually work within our plugin! > >> > > >> > Leaving aside the sheer rudeness of throwing unapproved ?patches? into > >> somebody?s code (without ever letting Ian know, for example) and resulting > >> in a broken function, I imagine that if looked into we would be able to > >> tweak our plugin code to actually work with the new jpeg library code. This > >> would be a Good Thing. If anyone knows enough about jpeg library, Debian, > >> blah blah blah to look into this then it might make a nice project that > >> would benefit the Raspberry Pi Scratch using world - several tens of > >> thousands of kids in the UK alone. > >> > > >> > >> This issue is tracked in Mantis 7539: "Plugins should use platform > >> libraries > >> where possible. Gentoo removed Squeak for this reason." > >> > >> http://bugs.squeak.org/view.php?id=7539 > >> > >> It was originally reported based on Gentoo rejecting Squeak, and it > >> affects Debian > >> and other Linux distributions that (quite reasonably) expect that > >> supported > >> applications should link to current versions of system libraries. > >> > >> If anyone is interested in working to improve this, please do. It would > >> be a great > >> way for someone to contribute a really useful update to the VM. > >> > >> Dave > >> > >> > >> > > A general idea would be to proceed in two steps: > > -1) upgrade the library in squeak svn repository, so as to fix the API > > evolutions mismatch > > -2) provide a way to link to existing library (via cmake/configure in > > VMMaker and apt/rpm package dependency at distrib level) > > > > Good plan, but what are the starting points? > > Which library exactly should be used? > > libjpeg - http://sourceforge.net/projects/libjpeg/ - which is the > > successor of the library used in squeak is licenced GPL v2, should we taint > > the whole VM? > > There is also an opened issue (http://sourceforge.net/p/libjpeg/bugs/5/ - > > 5 years old!!!) telling it does not compile on win32 (not sure which > > compiler though, it might be MSVC only), so this might be a hurdle for step > > 1). > > > > So what is the plan in details? > > > > Maybe http://www.openjpeg.org/ is a better match, BSD license... But it's > huge. The Mantis entry has links to the earlier discussions, so there is a good summary of the issues there. I do not know what the detailed plan should be but I think that we need two things: 1) Some platforms, probably including some unix platforms, need to use the old jpeg library code that we keep in platforms/Cross/plugins/JPEGReadWriter2Plugin. This must continue to work for the platforms that require it. 2) Other platforms, especially Linux distributions, would prefer to link directly to the jpeg runtime library provided by the operating system. There should be no GPL issues because linking to a runtime libjpeg is no different from linking to any other runtime library on Linux. It may be that the best way to achieve this is to provide different variants of jpeg plugin in VMMaker. But I have not looked at this in detail, so I cannot say what is the best approach. Dave |
In reply to this post by Nicolas Cellier
Thank you, Nicholas!
On Sat, May 10, 2014 at 6:46 AM, Nicolas Cellier <[hidden email]> wrote: > After further fixes i tested these forms successfully: > > > #( > pal_bk pal rgba8_bk rgba8 rgba16_bk rgba16 > rgb8_t_bk rgb8_t rgb16_t_bk rgb16_t > gray8a_bk gray8a gray16a_bk gray16a > gray8b_bk gray8b gray16b_bk gray16b > pal_bk_notrns palb > ) > do: [:e | > (ImageReadWriter formFromStream: > ('http://entropymine.com/jason/testbed/pngtrans/',e,'.png') asUrl > retrieveContents contentStream) asMorph openInWorld]. > > > > 2014-05-10 12:36 GMT+02:00 Nicolas Cellier > <[hidden email]>: > >> I just committed a fix. >> >> The PNGReadWriter currenlty has a hack to support 16 bits per channel gray >> scale (with a slow bit poker loop), but not with transparency... >> - transparency is handled with a palette >> - ColorForm only supports depth <= 8 >> Since the Form is forced to depth 8, and since we must choose an entry in >> the palette for transparency, I forced the transparency to be stored at 1st >> palette entry (value 0) in case of 16 bitsPerChannel. >> >> It seems to me that there was another bug even for bitsPerPixel <= 8: the >> palette is 1-based indexed so transparent color index was one off. >> >> It also seems to me that the inversion 255-pixelValue is wrong in the 16 >> bitsPerChannel bitPoker loop , I did change it. >> >> >> 2014-05-09 19:36 GMT+02:00 Yoshiki Ohshima <[hidden email]>: >> >>> On Fri, May 9, 2014 at 1:14 AM, Bert Freudenberg <[hidden email]> >>> wrote: >>> > Do we support any 16 bit-per-channel gifs? >>> >>> Not sure what you mean by "gifs" (I am specifically talking about >>> PNGs), but sure, I think Squeak should be able to read a PNG file, as >>> long as it is a valid one. If it is 16 bits per channel, it should >>> downscale the depth to 8 and create a 32 bpp Forms. This does happen >>> in most of the cases, but when the PNG file specifies the transparent >>> pixel (it is also in 16 bit value), it causes an error. >>> -- >>> -- Yoshiki >>> >> > > > > -- -- Yoshiki |
In reply to this post by David T. Lewis
2014-05-11 16:20 GMT+02:00 David T. Lewis <[hidden email]>:
Beware, this is why LGPL was created for, no? But GPL is different from LGPL. I don't feel like reading those licenses again, it's just too boring, so someone should confirm that: link with GPL and you become GPL, it's viral. It may be that the best way to achieve this is to provide different variants |
On Fri, May 16, 2014 at 10:51:17PM +0200, Nicolas Cellier wrote:
> 2014-05-11 16:20 GMT+02:00 David T. Lewis <[hidden email]>: > > > > The Mantis entry has links to the earlier discussions, so there is a good > > summary of the issues there. I do not know what the detailed plan should be > > but I think that we need two things: > > > > 1) Some platforms, probably including some unix platforms, need to use the > > old jpeg library code that we keep in > > platforms/Cross/plugins/JPEGReadWriter2Plugin. > > This must continue to work for the platforms that require it. > > > > 2) Other platforms, especially Linux distributions, would prefer to link > > directly to the jpeg runtime library provided by the operating system. > > > > There should be no GPL issues because linking to a runtime libjpeg is no > > different from linking to any other runtime library on Linux. > > > > > Beware, this is why LGPL was created for, no? > But GPL is different from LGPL. > I don't feel like reading those licenses again, it's just too boring, so > someone should confirm that: > link with GPL and you become GPL, it's viral. > Good catch. I don't know if it is a problem in this case, but I added a note to the Mantis issue so we do not forget to check. Thanks, Dave |
License
====== Sourceforge does not appear to host the current development of libjpeg, and seems to have caused us unnecessary worry about the license. The SF site has version 6b, which is 6 years behind Guido Vollbeding's version 9a. (I think SF may have trouble displaying licenses that aren't from a standard short list, so ended up with the wrong one.) Version 9a is at http://ijg.org/ Whether from Sourceforge or IJG or from Squeak's own repository[1], the license is not GPL. It is this custom but liberal license: http://directory.fsf.org/wiki?title=License:JPEG I am not a lawyer. I can't imagine incompatibilities with Squeak's Apache and MIT licenses. Compatibility ========= Ubuntu and Fedora prefer libjpeg-turbo, a fork which uses the same liberal license and, I hope, the same API. libjpeg-turbo really is on Sourceforge: http://sourceforge.net/p/libjpeg-turbo/code/HEAD/tree/trunk/ I am not in any way a C or SLANG hacker, though I would like to learn. Yet a quick browse of change.log in libjpegsr9a gives me some hope that the API has not had many breaking changes since version 6a, which I guess is the version that is in the Squeak platform sources. While I have them handy, here are the version numbers as I found them installed on my machines: Debian 7.1 libjpeg8 8d-1 (binary is libjpeg.so.8.4.0) Fedora 20 libjpeg-turbo 1.3.1 (binary is libjpeg.so.62.1.0) Ubuntu 12.04 (old I know) libjpeg-turbo 1.1.90+svn733-0ubuntu4.3 (binary is libjpeg.so.8.4.0) Guido's site links to this one, which hopefully will offer more specific clues about what Squeak would need to change to work with the newer platform libraries.. http://jpegclub.org/support/ Hope that helps. Have fun! David [1]: http://squeakvm.org/cgi-bin/viewvc.cgi/squeak/trunk/platforms/Cross/plugins/JPEGReadWriter2Plugin/ as the other David mentioned PS: It looks like the libjpeg README didn't make it into the Squeak repo, unless I looked in the wrong part of the tree. Would you like me to post a request for this in Mantis? |
David,
Thanks very much for this information. I have added it to the Mantis entry. Regarding the README for libjpeg, I think that the only README that should be added to the platforms source tree would be the one from 12 years ago that would match the actual source we are using in Cross. It's probably not worth worrying about at this point. Dave On Mon, May 19, 2014 at 08:33:11PM +0100, David Corking wrote: > License > ====== > > Sourceforge does not appear to host the current development of > libjpeg, and seems to have caused us unnecessary worry about the > license. The SF site has version 6b, which is 6 years behind Guido > Vollbeding's version 9a. (I think SF may have trouble displaying licenses > that aren't from a standard short list, so ended up with the wrong one.) > > Version 9a is at http://ijg.org/ > > Whether from Sourceforge or IJG or from Squeak's own repository[1], > the license is not GPL. It is this custom but liberal license: > > http://directory.fsf.org/wiki?title=License:JPEG > > I am not a lawyer. I can't imagine incompatibilities with Squeak's > Apache and MIT licenses. > > Compatibility > ========= > Ubuntu and Fedora prefer libjpeg-turbo, a fork which uses the same > liberal license and, I hope, the same API. > > libjpeg-turbo really is on Sourceforge: > > http://sourceforge.net/p/libjpeg-turbo/code/HEAD/tree/trunk/ > > I am not in any way a C or SLANG hacker, though I would like to learn. > Yet a quick browse of change.log in libjpegsr9a gives me some hope > that the API has not had many breaking changes since version 6a, which > I guess is the version that is in the Squeak platform sources. > > While I have them handy, here are the version numbers as I found them > installed on my machines: > Debian 7.1 libjpeg8 8d-1 (binary is libjpeg.so.8.4.0) > Fedora 20 libjpeg-turbo 1.3.1 (binary is libjpeg.so.62.1.0) > Ubuntu 12.04 (old I know) libjpeg-turbo 1.1.90+svn733-0ubuntu4.3 > (binary is libjpeg.so.8.4.0) > > Guido's site links to this one, which hopefully will offer more > specific clues about what Squeak would need to change to work with the > newer platform libraries.. > > http://jpegclub.org/support/ > > Hope that helps. Have fun! > > David > > [1]: http://squeakvm.org/cgi-bin/viewvc.cgi/squeak/trunk/platforms/Cross/plugins/JPEGReadWriter2Plugin/ > as the other David mentioned > > PS: > It looks like the libjpeg README didn't make it into the Squeak repo, > unless I looked in the wrong part of the tree. Would you like me to > post a request for this in Mantis? |
Free forum by Nabble | Edit this page |