From: Cosmin T. <co...@cs...> - 2006-04-24 16:42:38
|
Hi, Since now is a good time to apply binary-incompatible fixes, I suggest to fix a bunch of libpng types and type-related issues. (Sorry about the long message.) ** png_[u]int_32 ** First is png_uint_32 and png_int_32. Right now they're defined as: typedef unsigned long png_uint_32; typedef long png_int_32; I understand this definition is justified on 16-bit machines, but it's too wasteful on 64-bit machines, both in terms of space and CPU cycles. I propose to define it as: #if !defined(INT_MAX) || (INT_MAX < 0x7fffffffL) typedef unsigned long png_uint_32; typedef long png_int_32; #else typedef unsigned int png_uint_32; typedef int png_int_32; #endif limits.h is not currently included. But since libpng depends on zlib, and zlib can do it, it follows that libpng can do it. #ifndef PNG_NO_LIMITS_H # include <limits.h> #endif ** png_size_t and type casts ** A much more sensitive issue is png_size_t. It is right to define it based on size_t (as is currently done), but the problem lies in the way it's used. First of all, png_size_t has the following definition in pngconf.h: #ifdef PNG_SIZE_T typedef PNG_SIZE_T png_size_t; # define png_sizeof(x) png_convert_size(sizeof (x)) #else typedef size_t png_size_t; # define png_sizeof(x) sizeof (x) #endif This is wrong. Like png_uint_32 and friends, png_size_t needs not and should not be at the user's discretion. The right definition is: #ifdef PNG_NO_SIZE_T typdef unsigned int png_size_t; #else typedef size_t png_size_t; #endif #define png_sizeof(x) sizeof(x) (and no png_convert_size, please) Or, even better, remove png_sizeof completely, and consistently use sizeof everywhere. No C compiler, old or new, would break; on the other hand, there's the benefit of syntax highlight and other parsing features offered by various IDEs that recognize the 'sizeof' keyword. Here is the explanation: png_size_t is an important type that should not be tampered with. In a pre-ANSI compiler it's unsigned int, otherwise it's size_t, and it shouldn't be anything else. Furthermore, it should be used as much as possible where appropriate: to store row sizes, to iterate over rows, to be used as arguments of memcpy(), fread(), etc. and to retrieve their results, to retrieve the results of sizeof(), strlen(), etc. No larger type should be used: if the value is greater than what would fit in a png_size_t, the machine will not allow it. (I'm talking about 16-bit platforms in particular.) No narrower type should be used: otherwise, it will be pointlessly impossible to process very wide images (say (1<<30) pixels in width) on 64-bit machines, even if those machines allow it. Redefining png_size_t in pngconf.h is easy, but two more things must be done here: the consistent use of this type when handling all arrays and buffers (in particular, png_ptr->rowbytes and png_ptr->irowbytes), and the removal of as many as possible casts/conversions to and from other types like png_uint_32. And no png_convert_size(). About arrays and buffers: Not only rowbytes, irowbytes and png_get_rowbytes() should be png_size_t but also PNG_ROWBYTES should be defined as: #define PNG_ROWBYTES(pixel_bits, width) \ ((pixel_bits) >= 8 ? \ ((png_size_t)(width) * (((png_size_t)(pixel_bits)) >> 3)) : \ (( ((png_size_t)(width) * ((png_size_t)(pixel_bits))) + 7) >> 3) ) (BTW: why is the formula not simplified, by having +7 regardless of pixel_bits? The code would be still correct, and also smaller/faster.) Other change: png_get_compression_buffer_size() and png_set_compression_buffer_size() should operate on png_size_t, not png_uint_32. About type casts: I suppose the crazy casts to constants like (png_size_t)1 are there "just in case" that png_size_t might be some crazy user-defined type. I'd be glad to see the source code cleaned. But more importantly, type conversions between png_size_t and png_uint_32 should be minimized. The example below is a little exaggerated, because there is no real libpng code that has both var1 and var2 in the same context. I'm just using this example to make my point. void png_some_function(png_structp png_ptr, png_size_t some_size, png_charp some_string) { png_uint_32 var1 = some_size; png_uint_32 var2 = png_strlen(some_string); ... png_memcpy(some_ptr, other_ptr, (png_size_t)var1); png_memcpy(other_ptr, yet_another_ptr, (png_size_t)var2 + 1); } The right way to do it is by having var1 and var2 of type png_size_t. Using the casts to shut up the compiler warnings is bad. Assuming that png_uint_32 and png_size_t are compatible, the casts are pointless. Assuming that these types are not compatible, the compiler warnings are supposed to say so, and not be shut up. An allowed exception to this rule is: void png_other_function(png_structp png_ptr, png_bytep chunk_data, png_uint_32 chunk_len) { /* If we know that chunk_len is never larger than PNG_SIZE_MAX, * the cast is safe. If we don't know that, the code should be * modified accordingly. */ png_size_t length = (png_size_t)chunk_len; /* As much as possible, arguments to memcpy, fread, etc. * should never be cast. */ memcpy(png_ptr->blah, chunk_data, length); ... } There is a problem in the new context, however: png_malloc() returning png_uint_32. That's because of the need of 64K arrays on 16-bit systems. We could introduce a png_alloc_t, which would be png_uint_32 on 16-bit, and png_size_t everywhere else. Casts from png_alloc_t to png_size_t or png_uint_32 should be allowed, but they should be carefully managed. (Yes, we're introducing a new png_alloc_t type when we're supposed to simplify and clean up the code, but this one provides real type safety benefits, plus it enables 64-bit machines to process images of any size. ** other type casts ** I was talking about type casts, but png_size_t is not the only type that is excessivly cast. There are other pointless casts like: png_some_type *some_var; char some_string[]; png_charp other_string; ... do_something(png_ptr, (some_typep)some_var); // no cast is needed png_strcpy(some_string, (const char *)other_string); // same here etc. Conversions between png_charp and png_bytep are innocuous, but for style reasons, they should also be reduced to a minimum. NULLs, in particular, seem to be cast to a specially-designed constant that accompanies each pointer type. This is crazy: no pre-ANSI or post-ANSI compiler needs that; not even the 16-bit compilers that have to deal with "near" and "far" pointers! As a general rule of thumb, use as few casts as possible. If the compiler complains, the first thing to do is to change the types of local variables until they're right and the warnings are shut up. (That could literally be a fix and would avoid a potential crash somewhere!) Casts should be used only as a last resort, when real type fixes are not possible because of design limitations. And now is a good time to fix at least some design limitations ;-) ** Cheers, Cosmin |