[libav-devel] [PATCH 2/3] tiff: Add GeoTIFF support to the TIFF decoder

[Thomas Kühnel]

On 27.10.2011 23:44, Stefano Sabatini wrote:
> On date Sunday 2011-10-23 22:12:01 +0200, Thomas Kühnel encoded:
>> Updated.
[...]

>> +static double tget_double(const uint8_t **p, int le){
>> +    av_alias64 i = { .u64 = le ? AV_RL64(*p) : AV_RB64(*p)};
>> +    *p += 8;
>> +    return i.f64;
>> +}
> is type "double" guaranteed to be always 64 bits on all platforms?
>
Well doubles in a TIFF file are always 64 bit long, but there is nothing 
about the length of the double type in the C spec.

[...]
>> +
>> +static const char *search_keyval(const Key_name *keys, int n, int id) {
>> +    int low  = 0;
>> +    int high = n - 1;
>> +    while (low<= high) {
>> +        int mid = (low + high) / 2;
>> +        if (keys[mid].key>  id)
>> +            high = mid - 1;
>> +        else if (keys[mid].key<  id)
>> +            low = mid + 1;
>> +        else
>> +            return keys[mid].name;
>> +    }
>> +    return NULL;
>> +}
>
> what about bsearch()?
Oh, good to know that something like this already exists in the standard 
lib.

[...]
>>
>>   /** sizes of various TIFF field types (string size = 100)*/
>> -static const uint8_t type_sizes[6] = {
>> -    0, 1, 100, 2, 4, 8
>> +static const uint8_t type_sizes[14] = {
>> +    0, 1, 100, 2, 4, 8, 1, 1, 2, 4, 8, 4, 8, 4
>
> annotating this may greatly help readability, I mean:
>
> static const uint8_t type_sizes[14] = {
>      0,   // TIFF_SHORT
>      1,   // TIFF_LONG
>      100, // ...
>
>
Yeah, but I'd say this belongs to a separate patch.
>>   };
>>
>> +typedef struct TiffGeoKey {
>> +    int key;
>> +    int type;
>> +    int count;
>> +    int offset;
>> +    char *val;
>> +
>> +} TiffGeoKey;
>> +
>
>> +typedef struct Key_name {
>> +    const int key;
>> +    const char *const name;
>> +} Key_name;
>
> for overall consistency, "KeyName" is preferred
>
>> +
>> +enum TiffGeoKeys {
>
> nit: enum denotes a type, so the singular form looks more natural to
> me (TiffGeoKey)
>
>> +    TIFF_GT_MODEL_TYPE_GEOKEY                = 1024,
>
> maybe TIFF_GEOKEY_* =>  more grep friendly
The current form is more consistent with the spec.

[...]
>
>> +enum Geo_tiff_types {
>> +    GEOTIFF_SHORT  = 0,
>> +    GEOTIFF_DOUBLE = 34736,
>> +    GEOTIFF_STRING = 34737
>> +};
>> +
>> +typedef struct Geokey {
>> +    const char *const name;
>> +    const int type;
>> +} Geokey;
>
> what's exactly the relation between:
> Key_name
> enum TiffGeoKeys
> enum Geo_tiff_types
> Geokey
> ?
>
> maybe you can find a more consistent scheme
>
>>   /** sizes of various TIFF field types (string size = 1)*/
>> -static const uint8_t type_sizes2[6] = {
>> -    0, 1, 1, 2, 4, 8
>> +static const uint8_t type_sizes2[14] = {
>> +    0, 1, 1, 2, 4, 8, 1, 1, 2, 4, 8, 4, 8, 4
>>   };
>
> duplicated?
>
> What about to include tiff.h and avoid the information duplication
> (possibly in a separate patch).
The string-size is different for some reason, but otherwise this seems 
like a good idea.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-tiff-Add-GeoTIFF-support-to-the-TIFF-decoder.patch
Type: text/x-patch
Size: 89698 bytes
Desc: not available
URL: <http://lists.libav.org/pipermail/libav-devel/attachments/20111102/11ce4aff/attachment-0001.bin>