From: Michael K. <mi...@mu...> - 2008-11-26 18:52:43
|
On Wed, 26 Nov 2008, Donal K. Fellows wrote: > [Michael, please note that I'm not ranting at you specifically. But I'm > very very cross...] Aw, well I've been committed to TkPNG being good enough for a long time, but it's been out of my hands. :) > What's missing from the zlib API? If I know what to add/change, I can > fix the TIP today and we go to the vote. Clerical: * Jeff and Pascal both indicated they prefer Tcl_Zlib as a prefix (with unnamed others preferring Zlib_). [I don't care, but Tcl_Zlib makes more sense to me. Maybe you can put that to vote.] * 'compresseddata' in the Arguments section is not used anywhere and should be removed. 'data' should be changed to in/out, or (maybe better) change Put() to 'srcPtr' and Get() to 'dstPtr', and replace 'data' in Arguments with these. * I think the last time I read the TIP I was confused about Put/Get thinking one was for compression and the other decompression, because one inflate() or deflate() call both feeds data in and gets data out, when using zlib directly. If that's how Get/Put are intended to be used, it should be clarified. * Jeff mentioned in the previous thread something about consistency between "-limit" and "-count" at the Tcl level and about multiple compression method pairs versus one pair with options that don't look to have been addressed. But I only bring them up because I was skimming the thread and looking at what was changed since, not because I have an opinion. I'll just defer to Jeff et. al. regarding this stuff. You can look to the tcl-core archives for Oct 23-25 '08 for the thread in question. * I'm puzzled by the stream handle being an integer. I'd think it'd be an opaque pointer, but maybe this is just an implementation issue and the int is actually a hash table key. In any case, may be better as a typedef. (Only came up because I was wondering how to determine bytes consumed by Put(), and checking if TIP234 exposed the real zlib handle). Underspecified: * inflate() and deflate() may both return an error result. It's unspecified whether Zlib_StreamGet()/Put() are intended to return an error result (in which case, they should take an interp argument and handle error messages) or number of bytes read/written. * Get() should indicate that data is appended to the Tcl_Obj, or else take a starting offset. In the previous discussions, Pascal indicated he was leaning toward the former, and I can probably work with that (resetting Tcl_Obj length to 0 doesn't free memory, right? If so, it'd be a big performance hit to TkPNG; if not, should be fine). * A single call to inflate() or deflate() may, but will not necessarily, consume all provided input data or output buffer space. For Get(), if it always appends, this is fine. For Put(), it's unspecified what happens with the leftovers. I'm not certain you can simply keep calling inflate() or deflate() when the output buffer space is full and expect all input data to be consumed. So, does consumed input get trimmed from the Tcl_Obj so it can be passed again directly to consume more input? Does TIP234 buffer the remaining input itself, so the next Put() call adds the new data to what it's buffered for input? Or does it do neither, in which case Put() would need to provide the caller with info on how much was consumed, as well as take an additional offset argument. I can work with either of these options, but buffering would be more caller-friendly while offset input+count return would be most efficient. But it needs to be specified. Nice to Have: * It may be useful (particularly if, as I've suggested a couple times in the past, base64 decoding and images-as-strings are moved out of the GIF/PNG code into the generic photo code, turning both into channels and making it invisible to the format-specific handler) there were versions taking Tcl_Channels rather than Tcl_Objs. But it's not a requirement for me. If the underspecified things are specified, then I think I can make TkPNG work with it with minimal re-engineering. Currently, I can't tell enough from the TIP how it's used from C to say. -- Michael Kirkham President & CEO Muonics, Inc. http://www.muonics.com/ |