On date Sunday 2011-10-23 22:12:01 +0200, Thomas Kühnel encoded:
> Updated.

> From aa53f7c180fa08d0fcde29f743845550a0f9d972 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Thomas=20K=C3=BChnel?= <[email protected]>
> Date: Tue, 4 Oct 2011 00:19:52 +0200
> Subject: [PATCH 2/3] tiff: Add GeoTIFF support to the TIFF decoder
> 
> ---
>  libavcodec/Makefile    |    4 +-
>  libavcodec/tiff.c      |  336 +++++++++
>  libavcodec/tiff.h      |   91 +++-
>  libavcodec/tiff_data.c | 1824 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/tiff_data.h |   85 +++
>  libavcodec/tiffenc.c   |    4 +-
>  6 files changed, 2338 insertions(+), 6 deletions(-)
>  create mode 100644 libavcodec/tiff_data.c
>  create mode 100644 libavcodec/tiff_data.h
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index bd4275a..cb2c227 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -367,8 +367,8 @@ OBJS-$(CONFIG_TARGA_ENCODER)           += targaenc.o rle.o
>  OBJS-$(CONFIG_THEORA_DECODER)          += xiph.o
>  OBJS-$(CONFIG_THP_DECODER)             += mjpegdec.o mjpeg.o
>  OBJS-$(CONFIG_TIERTEXSEQVIDEO_DECODER) += tiertexseqv.o
> -OBJS-$(CONFIG_TIFF_DECODER)            += tiff.o lzw.o faxcompr.o
> -OBJS-$(CONFIG_TIFF_ENCODER)            += tiffenc.o rle.o lzwenc.o
> +OBJS-$(CONFIG_TIFF_DECODER)            += tiff.o lzw.o faxcompr.o tiff_data.o
> +OBJS-$(CONFIG_TIFF_ENCODER)            += tiffenc.o rle.o lzwenc.o 
> tiff_data.o
>  OBJS-$(CONFIG_TMV_DECODER)             += tmv.o cga_data.o
>  OBJS-$(CONFIG_TRUEMOTION1_DECODER)     += truemotion1.o
>  OBJS-$(CONFIG_TRUEMOTION2_DECODER)     += truemotion2.o
> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> index a88d0f9..20ffb13 100644
> --- a/libavcodec/tiff.c
> +++ b/libavcodec/tiff.c
> @@ -31,10 +31,12 @@
>  #endif
>  #include "lzw.h"
>  #include "tiff.h"
> +#include "tiff_data.h"
>  #include "faxcompr.h"
>  #include "libavutil/common.h"
>  #include "libavutil/intreadwrite.h"
>  #include "libavutil/imgutils.h"
> +#include "libavutil/avstring.h"
>  
>  typedef struct TiffContext {
>      AVCodecContext *avctx;
> @@ -57,6 +59,9 @@ typedef struct TiffContext {
>      const uint8_t* stripsizes;
>      int stripsize, stripoff;
>      LZWState *lzw;
> +
> +    int geokeycount;
> +    TiffGeoKey *geokeys;
>  } TiffContext;
>  
>  static int tget_short(const uint8_t **p, int le){
> @@ -71,6 +76,12 @@ static int tget_long(const uint8_t **p, int le){
>      return v;
>  }
>  
> +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?


>  static int tget(const uint8_t **p, int type, int le){
>      switch(type){
>      case TIFF_BYTE : return *(*p)++;
> @@ -80,6 +91,211 @@ static int tget(const uint8_t **p, int type, int le){
>      }
>  }
>  
> +static void free_geokeys(TiffContext * const s) {
> +    int i;
> +    for (i = 0; i < s->geokeycount; i++) {
> +        if (s->geokeys[i].val)
> +            av_freep(&s->geokeys[i].val);
> +    }
> +    av_freep(&s->geokeys);
> +}
> +
> +static const char *get_geokeyname(int key) {

nit: geokey_name() (more readable)

> +    if (key >= TIFF_VERT_KEY_ID_OFFSET &&
> +        key - TIFF_VERT_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_vertkeys))
> +        return ff_tiff_vertkeys[key - TIFF_VERT_KEY_ID_OFFSET].name;
> +    if (key >= TIFF_PROJ_KEY_ID_OFFSET &&
> +        key - TIFF_PROJ_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_projkeys))
> +        return ff_tiff_projkeys[key - TIFF_PROJ_KEY_ID_OFFSET].name;
> +    if (key >= TIFF_GEOG_KEY_ID_OFFSET &&
> +        key - TIFF_GEOG_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_geogkeys))
> +        return ff_tiff_geogkeys[key - TIFF_GEOG_KEY_ID_OFFSET].name;
> +    if (key >= TIFF_CONF_KEY_ID_OFFSET &&
> +        key - TIFF_CONF_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_confkeys))
> +        return ff_tiff_confkeys[key - TIFF_CONF_KEY_ID_OFFSET].name;
> +    else
> +        return NULL;
> +}
> +
> +static int get_geokeytype(int key) {

nit: get_geokey_type()

> +    if (key >= TIFF_VERT_KEY_ID_OFFSET &&
> +        key - TIFF_VERT_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_vertkeys))
> +        return ff_tiff_vertkeys[key - TIFF_VERT_KEY_ID_OFFSET].type;
> +    if (key >= TIFF_PROJ_KEY_ID_OFFSET &&
> +        key - TIFF_PROJ_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_projkeys))
> +        return ff_tiff_projkeys[key - TIFF_PROJ_KEY_ID_OFFSET].type;
> +    if (key >= TIFF_GEOG_KEY_ID_OFFSET &&
> +        key - TIFF_GEOG_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_geogkeys))
> +        return ff_tiff_geogkeys[key - TIFF_GEOG_KEY_ID_OFFSET].type;

> +    if (key >= TIFF_CONF_KEY_ID_OFFSET &&
> +        key - TIFF_CONF_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_confkeys))
> +        return ff_tiff_confkeys[key - TIFF_CONF_KEY_ID_OFFSET].type;

Note: you may adopt a macro here for readability/refactoring purposes.

#define RET_GEOKEY(TYPE, array) \
    if (key >= TIFF_##TYPE##_KEY_ID_OFFSET &&
        key - TIFF_##TYPE##_KEY_ID_OFFSET < FF_ARRAY_ELEMS(ff_tiff_##array))
        return ff_tiff_array##_keys[key - TIFF_##TYPE##_KEY_ID_OFFSET].type;

> +   return -1

AVERROR_INVALIDDATA or a better error code is preferable

> +}
> +
> +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()?

> +static char *get_geokeyval(int key, int val) {
> +    char *ap;
> +
> +    if (val == TIFF_GEO_KEY_UNDEFINED)
> +        return av_strdup("undefined");
> +    if (val == TIFF_GEO_KEY_USER_DEFINED)
> +        return av_strdup("User-Defined");
> +
> +    switch (key) {
> +    case TIFF_GT_MODEL_TYPE_GEOKEY:
> +        if (val >= TIFF_GTMODELTYPE_OFFSET &&
> +            val - TIFF_GTMODELTYPE_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_gtmodeltypecodes))
> +            return av_strdup(ff_tiff_gtmodeltypecodes[val - 
> TIFF_GTMODELTYPE_OFFSET]);
> +        break;
> +    case TIFF_GT_RASTER_TYPE_GEOKEY:
> +        if (val >= TIFF_RASTERTYPE_OFFSET &&
> +            val - TIFF_RASTERTYPE_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_rastertypecodes))
> +            return av_strdup(ff_tiff_rastertypecodes[val - 
> TIFF_RASTERTYPE_OFFSET]);
> +        break;
> +    case TIFF_GEOG_LINEAR_UNITS_GEOKEY:
> +    case TIFF_PROJ_LINEAR_UNITS_GEOKEY:
> +    case TIFF_VERTICAL_UNITS_GEOKEY:
> +        if (val >= TIFF_LINEARUNIT_OFFSET &&
> +            val - TIFF_LINEARUNIT_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_linearunitcodes))
> +            return av_strdup(ff_tiff_linearunitcodes[val - 
> TIFF_LINEARUNIT_OFFSET]);
> +        break;
> +    case TIFF_GEOG_ANGULAR_UNITS_GEOKEY:
> +    case TIFF_GEOG_AZIMUTH_UNITS_GEOKEY:
> +        if (val >= TIFF_ANGULARUNIT_OFFSET &&
> +            val - TIFF_ANGULARUNIT_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_angularunitcodes))
> +            return av_strdup(ff_tiff_angularunitcodes[val - 
> TIFF_ANGULARUNIT_OFFSET]);
> +        break;
> +    case TIFF_GEOGRAPHIC_TYPE_GEOKEY:
> +        if (val >= TIFF_GCSTYPE_OFFSET &&
> +            val - TIFF_GCSTYPE_OFFSET < FF_ARRAY_ELEMS(ff_tiff_gcstypecodes))
> +            return av_strdup(ff_tiff_gcstypecodes[val - 
> TIFF_GCSTYPE_OFFSET]);
> +        if (val >= TIFF_GCSETYPE_OFFSET &&
> +            val - TIFF_GCSETYPE_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_gcsetypecodes))
> +            return av_strdup(ff_tiff_gcsetypecodes[val - 
> TIFF_GCSETYPE_OFFSET]);
> +        break;
> +    case TIFF_GEOG_GEODETIC_DATUM_GEOKEY:
> +        if (val >= TIFF_GEODETICDATUM_OFFSET &&
> +            val - TIFF_GEODETICDATUM_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_geodeticdatumcodes))
> +            return av_strdup(ff_tiff_geodeticdatumcodes[val - 
> TIFF_GEODETICDATUM_OFFSET]);
> +        if (val - TIFF_GEODETICDATUME_OFFSET >= 0 &&
> +            val - TIFF_GEODETICDATUME_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_geodeticdatumecodes))
> +            return av_strdup(ff_tiff_geodeticdatumecodes[val - 
> TIFF_GEODETICDATUME_OFFSET]);
> +        break;
> +    case TIFF_GEOG_ELLIPSOID_GEOKEY:
> +        if (val >= TIFF_ELLIPSOID_OFFSET &&
> +            val - TIFF_ELLIPSOID_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_ellipsoidcodes))
> +            return av_strdup(ff_tiff_ellipsoidcodes[val - 
> TIFF_ELLIPSOID_OFFSET]);
> +        break;
> +    case TIFF_GEOG_PRIME_MERIDIAN_GEOKEY:
> +        if (val >= TIFF_PRIMEMERIDIAN_OFFSET &&
> +            val - TIFF_PRIMEMERIDIAN_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_primemeridiancodes))
> +            return av_strdup(ff_tiff_primemeridiancodes[val - 
> TIFF_PRIMEMERIDIAN_OFFSET]);
> +        break;
> +    case TIFF_PROJECTED_CS_TYPE_GEOKEY:
> +        return av_strdup(search_keyval(ff_tiff_proj_cs_type_codes, 
> FF_ARRAY_ELEMS(ff_tiff_proj_cs_type_codes), val));
> +        break;
> +    case TIFF_PROJECTION_GEOKEY:
> +        return av_strdup(search_keyval(ff_tiff_projection_codes, 
> FF_ARRAY_ELEMS(ff_tiff_projection_codes), val));
> +        break;
> +    case TIFF_PROJ_COORD_TRANS_GEOKEY:
> +        if (val >= COORD_TRANS_OFFSET &&
> +            val - COORD_TRANS_OFFSET < 
> FF_ARRAY_ELEMS(ff_tiff_coord_trans_codes))
> +            return av_strdup(ff_tiff_coord_trans_codes[val - 
> COORD_TRANS_OFFSET]);
> +        break;
> +    }

Note: again, maybe a macro would simplify reading the code

> +    ap = av_malloc(14);
> +    if (ap)
> +        snprintf(ap, 14, "Unknown-%d", val);
> +    return ap;
> +}
> +
> +static char *doubles2str(double *dp, int count, const char *sep) {
> +    int i;
> +    char *ap;

> +    if(!sep) sep = ", ";

Nit+: if_(...)

> +    ap = av_malloc((15 + strlen(sep)) * count);
> +    if (!ap)
> +        return NULL;
> +    ap[0] = '\0';

> +    for (i = 0; i < count; i++)
> +        snprintf(&ap[strlen(ap)], 15 + strlen(sep), "%f%s", dp[i], sep);

more efficient:

    for (i = 0; i < count; i++) {
        int l = snprintf(&ap, 15 + strlen(sep), "%f%s", dp[i], sep);
        ap += l;
    }

(then returns ap0)
not that efficiency matters here...

> +    ap[strlen(ap) - strlen(sep)] = '\0';
> +    return ap;
> +}
> +

> +static char *shorts2str(int *sp, int count, const char *sep) {
> +    int i;
> +    char *ap;
> +    if(!sep) sep = ", ";
> +    ap = av_malloc((5 + strlen(sep)) * count);
> +    if (!ap)
> +        return NULL;
> +    ap[0] = '\0';
> +    for (i = 0; i < count; i++)
> +        snprintf(&ap[strlen(ap)], 5 + strlen(sep), "%d%s", sp[i], sep);
> +    ap[strlen(ap) - strlen(sep)] = '\0';
> +    return ap;
> +}

idem

> +static int add_doubles_metadata(const uint8_t **buf, int count, const char 
> *name, const char *sep, TiffContext *s) {

nit: maybe split the long signature, here and below.
nit: K&R style is (function open brace on a separate line):
SIG
{
...
}

> +    char * ap;
> +    int i;
> +    double *dp = av_malloc(count * type_sizes[TIFF_DOUBLE]);
> +    if (!dp)
> +        return AVERROR(ENOMEM);
> +
> +    for (i = 0; i < count; i++)
> +        dp[i] = tget_double(buf, s->le);
> +    ap = doubles2str(dp, count, sep);
> +    av_freep(&dp);
> +    if (!ap)
> +        return AVERROR(ENOMEM);
> +    av_dict_set(&s->picture.metadata, name, ap, AV_DICT_DONT_STRDUP_VAL);
> +    return 0;
> +}
> +
> +static int add_shorts_metadata(const uint8_t **buf, int count, const char 
> *name, const char *sep, TiffContext *s) {
> +    char * ap;
> +    int i;
> +    int *sp = av_malloc(count * type_sizes[TIFF_DOUBLE]);
> +    if (!sp)
> +        return AVERROR(ENOMEM);
> +
> +    for (i = 0; i < count; i++)
> +        sp[i] = tget_short(buf, s->le);
> +    ap = shorts2str(sp, count, sep);
> +    av_freep(&sp);
> +    if (!ap)
> +        return AVERROR(ENOMEM);
> +    av_dict_set(&s->picture.metadata, name, ap, AV_DICT_DONT_STRDUP_VAL);
> +    return 0;
> +}
> +
> +static int add_metadata(const uint8_t **buf, int count, int type, const char 
> *name, const char *sep, TiffContext *s) {
> +    switch(type) {
> +    case TIFF_DOUBLE: return add_doubles_metadata(buf, count, name, sep, s);
> +    case TIFF_SHORT : return add_shorts_metadata(buf, count, name, sep, s);

> +    default         : return -1;

nit: AVERROR_INVALIDDATA?

> +    };
> +}
> +
>  #if CONFIG_ZLIB
>  static int tiff_uncompress(uint8_t *dst, unsigned long *len, const uint8_t 
> *src, int size)
>  {
> @@ -281,6 +497,7 @@ static int tiff_decode_tag(TiffContext *s, const uint8_t 
> *start, const uint8_t *
>      int i, j;
>      uint32_t *pal;
>      const uint8_t *rp, *gp, *bp;
> +    double *dp;
>  
>      if (end_buf - buf < 12)
>          return -1;
> @@ -485,6 +702,99 @@ static int tiff_decode_tag(TiffContext *s, const uint8_t 
> *start, const uint8_t *
>          if(s->compr == TIFF_G4)
>              s->fax_opts = value;
>          break;

> +    case TIFF_MODEL_TIEPOINT:
> +        if (add_metadata(&buf, count, type, "ModelTiepointTag", NULL, s)) {
> +            av_log(s->avctx, AV_LOG_ERROR, "Error allocating temporary 
> buffer\n");
> +            return AVERROR(ENOMEM);
> +        }
> +        break;
> +    case TIFF_MODEL_PIXEL_SCALE:
> +        if (add_metadata(&buf, count, type, "ModelPixelScaleTag", NULL, s)) {
> +            av_log(s->avctx, AV_LOG_ERROR, "Error allocating temporary 
> buffer\n");
> +            return AVERROR(ENOMEM);
> +        }
> +        break;
> +    case TIFF_MODEL_TRANSFORMATION:
> +        if (add_metadata(&buf, count, type, "ModelTransformationTag", NULL, 
> s)) {
> +            av_log(s->avctx, AV_LOG_ERROR, "Error allocating temporary 
> buffer\n");
> +            return AVERROR(ENOMEM);
> +        }
> +        break;
> +    case TIFF_GEO_KEY_DIRECTORY:
> +        if (add_metadata(&buf, 1, type, "GeoTIFF_Version", NULL, s)) {
> +            av_log(s->avctx, AV_LOG_ERROR, "Error allocating temporary 
> buffer\n");
> +            return AVERROR(ENOMEM);
> +        }
> +        if (add_metadata(&buf, 2, type, "GeoTIFF_Key_Revision", ".", s)) {
> +            av_log(s->avctx, AV_LOG_ERROR, "Error allocating temporary 
> buffer\n");
> +            return AVERROR(ENOMEM);
> +        }

if ((ret = add_metadata(...) < 0) {
   ...
   return ret;
}

the problem is not necessarily an allocation error, but depends on the
called routine internal logic. You may also factorize this code with evil 
macroisms
or with a goto metadata_error.

> +        s->geokeycount   = tget_short(&buf, s->le);
> +        if ( s->geokeycount > count / 4 - 1) {
> +            s->geokeycount = count / 4 - 1;
> +            av_log(s->avctx, AV_LOG_WARNING, "GeoTIFF key directory buffer 
> shorter than specified\n");
> +        }
> +        s->geokeys       = av_mallocz(sizeof(TiffGeoKey) * s->geokeycount);
> +        if (!s->geokeys) {
> +            av_log(s->avctx, AV_LOG_ERROR, "Error allocating temporary 
> buffer\n");
> +            return AVERROR(ENOMEM);
> +        }
> +        for (i = 0; i < s->geokeycount; i++) {
> +            s->geokeys[i].key    = tget_short(&buf, s->le);
> +            s->geokeys[i].type   = tget_short(&buf, s->le);
> +            s->geokeys[i].count  = tget_short(&buf, s->le);
> +
> +            if (!s->geokeys[i].type)
> +                s->geokeys[i].val  = get_geokeyval(s->geokeys[i].key, 
> tget_short(&buf, s->le));
> +            else
> +                s->geokeys[i].offset = tget_short(&buf, s->le);
> +        }
> +        break;
> +    case TIFF_GEO_DOUBLE_PARAMS:
> +        dp = av_malloc(count * type_sizes[TIFF_DOUBLE]);
> +        if (!dp) {
> +            av_log(s->avctx, AV_LOG_ERROR, "Error allocating temporary 
> buffer\n");
> +            return AVERROR(ENOMEM);
> +        }
> +        for (i = 0; i < count; i++)
> +            dp[i] = tget_double(&buf, s->le);
> +        for (i = 0; i < s->geokeycount; i++) {
> +            if (s->geokeys[i].type == TIFF_GEO_DOUBLE_PARAMS) {
> +                if (s->geokeys[i].count == 0
> +                    || s->geokeys[i].offset + s->geokeys[i].count > count)
> +                    av_log(s->avctx, AV_LOG_WARNING, "Invalid GeoTIFF key 
> %d\n", s->geokeys[i].key);
> +                else {
> +                    char *ap = doubles2str(&dp[s->geokeys[i].offset], 
> s->geokeys[i].count, ", ");
> +                    if (!ap) {
> +                        av_log(s->avctx, AV_LOG_ERROR, "Error allocating 
> temporary buffer\n");
> +                        av_freep(&dp);
> +                        return AVERROR(ENOMEM);
> +                    }
> +                    s->geokeys[i].val = ap;
> +                }
> +            }
> +        }
> +        av_freep(&dp);
> +        break;
> +    case TIFF_GEO_ASCII_PARAMS:
> +        for (i = 0; i < s->geokeycount; i++) {
> +            if (s->geokeys[i].type == TIFF_GEO_ASCII_PARAMS) {
> +                if (s->geokeys[i].count == 0
> +                    || s->geokeys[i].offset +  s->geokeys[i].count > count)
> +                    av_log(s->avctx, AV_LOG_WARNING, "Invalid GeoTIFF key 
> %d\n", s->geokeys[i].key);
> +                else {
> +                    char *ap = av_malloc(s->geokeys[i].count);
> +                    if (!ap) {
> +                        av_log(s->avctx, AV_LOG_ERROR, "Error allocating 
> temporary buffer\n");
> +                        return AVERROR(ENOMEM);
> +                    }
> +                    memcpy(ap, &buf[s->geokeys[i].offset], 
> s->geokeys[i].count);
> +                    ap[s->geokeys[i].count - 1] = '\0'; //replace the "|" 
> delimiter with a 0 byte
> +                    s->geokeys[i].val = ap;
> +                }
> +            }
> +        }
> +        break;
>      default:
>          av_log(s->avctx, AV_LOG_DEBUG, "Unknown or unsupported tag 
> %d/0X%0X\n", tag, tag);
>      }
> @@ -522,6 +832,10 @@ static int decode_frame(AVCodecContext *avctx,
>      s->invert = 0;
>      s->compr = TIFF_RAW;
>      s->fill_order = 0;
> +    free_geokeys(s);
> +    /* free existing metadata */
> +    av_dict_free(&s->picture.metadata);
> +
>      // As TIFF 6.0 specification puts it "An arbitrary but carefully chosen 
> number
>      // that further identifies the file as a TIFF file"
>      if(tget_short(&buf, le) != 42){
> @@ -541,6 +855,24 @@ static int decode_frame(AVCodecContext *avctx,
>              return -1;
>          buf += 12;
>      }
> +
> +    for (i = 0; i<s->geokeycount; i++) {
> +        const char *keyname = get_geokeyname(s->geokeys[i].key);
> +        if (!keyname) {
> +            av_log(avctx, AV_LOG_WARNING, "Unknown or unsupported GeoTIFF 
> key %d\n", s->geokeys[i].key);
> +            continue;
> +        }
> +        if (get_geokeytype(s->geokeys[i].key) != s->geokeys[i].type) {
> +            av_log(avctx, AV_LOG_WARNING, "Type of GeoTIFF key %d is 
> wrong\n", s->geokeys[i].key);
> +            continue;
> +        }
> +        ret = av_dict_set(&s->picture.metadata, keyname, s->geokeys[i].val, 
> 0);
> +        if (ret<0) {
> +            av_log(avctx, AV_LOG_ERROR, "Writing metadata failed\n");
> +            return ret;
> +        }
> +    }
> +
>      if(!s->stripdata && !s->stripoff){
>          av_log(avctx, AV_LOG_ERROR, "Image data is missing\n");
>          return -1;
> @@ -624,6 +956,10 @@ static av_cold int tiff_end(AVCodecContext *avctx)
>  {
>      TiffContext * const s = avctx->priv_data;
>  
> +    free_geokeys(s);
> +    if(avctx->coded_frame && avctx->coded_frame->metadata)
> +        av_dict_free(&avctx->coded_frame->metadata);
> +
>      ff_lzw_decode_close(&s->lzw);
>      if(s->picture.data[0])
>          avctx->release_buffer(avctx, &s->picture);
> diff --git a/libavcodec/tiff.h b/libavcodec/tiff.h
> index cf890d6..8e6e0be 100644
> --- a/libavcodec/tiff.h
> +++ b/libavcodec/tiff.h
> @@ -58,6 +58,12 @@ enum TiffTags{
>      TIFF_YCBCR_SUBSAMPLING = 0x212,
>      TIFF_YCBCR_POSITIONING = 0x213,
>      TIFF_REFERENCE_BW = 0x214,
> +    TIFF_MODEL_TIEPOINT = 0x8482,
> +    TIFF_MODEL_PIXEL_SCALE = 0x830E,
> +    TIFF_MODEL_TRANSFORMATION = 0x8480,
> +    TIFF_GEO_KEY_DIRECTORY = 0x87AF,
> +    TIFF_GEO_DOUBLE_PARAMS = 0x87B0,
> +    TIFF_GEO_ASCII_PARAMS = 0x87B1
>  };
>  
>  /** list of TIFF compression types */
> @@ -80,11 +86,92 @@ enum TiffTypes{
>      TIFF_SHORT,
>      TIFF_LONG,
>      TIFF_RATIONAL,
> +    TIFF_SBYTE,
> +    TIFF_UNDEFINED,
> +    TIFF_SSHORT,
> +    TIFF_SLONG,
> +    TIFF_SRATIONAL,
> +    TIFF_FLOAT,
> +    TIFF_DOUBLE,
> +    TIFF_IFD
>  };
>  
>  /** 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, // ...


>  };
>  
> +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

> +    TIFF_GT_RASTER_TYPE_GEOKEY               = 1025,
> +    TIFF_GT_CITATION_GEOKEY                  = 1026,
> +    TIFF_GEOGRAPHIC_TYPE_GEOKEY              = 2048,
> +    TIFF_GEOG_CITATION_GEOKEY                = 2049,
> +    TIFF_GEOG_GEODETIC_DATUM_GEOKEY          = 2050,
> +    TIFF_GEOG_PRIME_MERIDIAN_GEOKEY          = 2051,
> +    TIFF_GEOG_LINEAR_UNITS_GEOKEY            = 2052,
> +    TIFF_GEOG_LINEAR_UNIT_SIZE_GEOKEY        = 2053,
> +    TIFF_GEOG_ANGULAR_UNITS_GEOKEY           = 2054,
> +    TIFF_GEOG_ANGULAR_UNIT_SIZE_GEOKEY       = 2055,
> +    TIFF_GEOG_ELLIPSOID_GEOKEY               = 2056,
> +    TIFF_GEOG_SEMI_MAJOR_AXIS_GEOKEY         = 2057,
> +    TIFF_GEOG_SEMI_MINOR_AXIS_GEOKEY         = 2058,
> +    TIFF_GEOG_INV_FLATTENING_GEOKEY          = 2059,
> +    TIFF_GEOG_AZIMUTH_UNITS_GEOKEY           = 2060,
> +    TIFF_GEOG_PRIME_MERIDIAN_LONG_GEOKEY     = 2061,
> +    TIFF_PROJECTED_CS_TYPE_GEOKEY            = 3072,
> +    TIFF_PCS_CITATION_GEOKEY                 = 3073,
> +    TIFF_PROJECTION_GEOKEY                   = 3074,
> +    TIFF_PROJ_COORD_TRANS_GEOKEY             = 3075,
> +    TIFF_PROJ_LINEAR_UNITS_GEOKEY            = 3076,
> +    TIFF_PROJ_LINEAR_UNIT_SIZE_GEOKEY        = 3077,
> +    TIFF_PROJ_STD_PARALLEL1_GEOKEY           = 3078,
> +    TIFF_PROJ_STD_PARALLEL2_GEOKEY           = 3079,
> +    TIFF_PROJ_NAT_ORIGIN_LONG_GEOKEY         = 3080,
> +    TIFF_PROJ_NAT_ORIGIN_LAT_GEOKEY          = 3081,
> +    TIFF_PROJ_FALSE_EASTING_GEOKEY           = 3082,
> +    TIFF_PROJ_FALSE_NORTHING_GEOKEY          = 3083,
> +    TIFF_PROJ_FALSE_ORIGIN_LONG_GEOKEY       = 3084,
> +    TIFF_PROJ_FALSE_ORIGIN_LAT_GEOKEY        = 3085,
> +    TIFF_PROJ_FALSE_ORIGIN_EASTING_GEOKEY    = 3086,
> +    TIFF_PROJ_FALSE_ORIGIN_NORTHING_GEOKEY   = 3087,
> +    TIFF_PROJ_CENTER_LONG_GEOKEY             = 3088,
> +    TIFF_PROJ_CENTER_LAT_GEOKEY              = 3089,
> +    TIFF_PROJ_CENTER_EASTING_GEOKEY          = 3090,
> +    TIFF_PROJ_CENTER_NORTHING_GEOKEY         = 3091,
> +    TIFF_PROJ_SCALE_AT_NAT_ORIGIN_GEOKEY     = 3092,
> +    TIFF_PROJ_SCALE_AT_CENTER_GEOKEY         = 3093,
> +    TIFF_PROJ_AZIMUTH_ANGLE_GEOKEY           = 3094,
> +    TIFF_PROJ_STRAIGHT_VERT_POLE_LONG_GEOKEY = 3095,
> +    TIFF_VERTICAL_CS_TYPE_GEOKEY             = 4096,
> +    TIFF_VERTICAL_CITATION_GEOKEY            = 4097,
> +    TIFF_VERTICAL_DATUM_GEOKEY               = 4098,
> +    TIFF_VERTICAL_UNITS_GEOKEY               = 4099
> +};
> +

> +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

> +
>  #endif /* AVCODEC_TIFF_H */
> diff --git a/libavcodec/tiff_data.c b/libavcodec/tiff_data.c
> new file mode 100644
> index 0000000..e562832
> --- /dev/null
> +++ b/libavcodec/tiff_data.c
> @@ -0,0 +1,1824 @@
> +/*
> + * TIFF data tables
> + * Copyright (c) 2011 Thomas Kuehnel
> + *
> + * This file is part of Libav.
> + *
> + * Libav is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * Libav is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with Libav; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
> + */
> +
> +/**
> + * @file
> + * TIFF data tables
> + * @author Thomas Kuehnel
> + */
> +
> +#include "tiff_data.h"

Please put a link to the specs if you have one.

> +
> +const Geokey ff_tiff_confkeys[] = {

Nit: ff_tiff_conf_keys is preferred (more readable, especially on non
fluent English readers), unless this is for spec consistency sake.

Same below.

> +const Geokey ff_tiff_geogkeys[] = {
[...]
> +const Geokey ff_tiff_projkeys[] = {
[...]
> +const Geokey ff_tiff_vertkeys[] = {
[...]
> +const char *const ff_tiff_gtmodeltypecodes[] = {
[...]
> +const char *const ff_tiff_rastertypecodes[] = {
[...]
> +const char *const ff_tiff_linearunitcodes[] = {
> +    "Linear_Meter",
> +    "Linear_Foot",
> +    "Linear_Foot_US_Survey",
> +    "Linear_Foot_Modified_American",
> +    "Linear_Foot_Clarke",
> +    "Linear_Foot_Indian",
> +    "Linear_Link",
> +    "Linear_Link_Benoit",
> +    "Linear_Link_Sears",
> +    "Linear_Chain_Benoit",
> +    "Linear_Chain_Sears",
> +    "Linear_Yard_Sears",
> +    "Linear_Yard_Indian",
> +    "Linear_Fathom",
> +    "Linear_Mile_International_Nautical"
> +};

funny

> +
> +const char *const ff_tiff_angularunitcodes[] = {
[...]
> +const char *const ff_tiff_gcstypecodes[] = {
[...]
> +const char *const ff_tiff_gcsetypecodes[] = {
[...]
> +const char *const ff_tiff_geodeticdatumcodes[] = {
[...]
> +const char *const ff_tiff_geodeticdatumecodes[] = {
[...]
> +const char *const ff_tiff_ellipsoidcodes[] = {
[...]
> +const char *const ff_tiff_primemeridiancodes[] = {
[...]
> +const Key_name ff_tiff_proj_cs_type_codes[] = {
[...]
> +const Key_name ff_tiff_projection_codes[] = {
[...]
> +const char *const ff_tiff_coord_trans_codes[] = {
[...]

Uhm, many of these will need updating after each main geopolitical
trouble / natural catastrophe.

> diff --git a/libavcodec/tiff_data.h b/libavcodec/tiff_data.h
> new file mode 100644
> index 0000000..f8817d2
> --- /dev/null
> +++ b/libavcodec/tiff_data.h
> @@ -0,0 +1,85 @@
> +/*
> + * TIFF data tables
> + * Copyright (c) 2011 Thomas Kuehnel
> + *
> + * This file is part of Libav.
> + *
> + * Libav is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * Libav is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with Libav; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
> + */
> +
> +/**
> + * @file
> + * TIFF data tables
> + * @author Thomas Kuehnel

again, please add a link to the (official) spec

> + */
> +
> +#ifndef AVCODEC_TIFF_DATA_H
> +#define AVCODEC_TIFF_DATA_H
> +
> +#include "tiff.h"
> +
> +#define TIFF_CONF_KEY_ID_OFFSET 1024
> +extern const Geokey ff_tiff_confkeys[3];
> +
> +#define TIFF_GEOG_KEY_ID_OFFSET 2048
> +extern const Geokey ff_tiff_geogkeys[14];
> +
> +#define TIFF_PROJ_KEY_ID_OFFSET 3072
> +extern const Geokey ff_tiff_projkeys[24];
> +
> +#define TIFF_VERT_KEY_ID_OFFSET 4096
> +extern const Geokey ff_tiff_vertkeys[4];
> +
> +#define TIFF_GEO_KEY_UNDEFINED    0
> +#define TIFF_GEO_KEY_USER_DEFINED 32767
> +
> +#define TIFF_GTMODELTYPE_OFFSET 1
> +extern const char *const ff_tiff_gtmodeltypecodes[3];
> +
> +#define TIFF_RASTERTYPE_OFFSET 1
> +extern const char *const ff_tiff_rastertypecodes[2];
> +
> +#define TIFF_LINEARUNIT_OFFSET 9001
> +extern const char *const ff_tiff_linearunitcodes[15];
> +
> +#define TIFF_ANGULARUNIT_OFFSET 9101
> +extern const char *const ff_tiff_angularunitcodes[8];
> +
> +#define TIFF_GCSTYPE_OFFSET 4201
> +extern const char *const ff_tiff_gcstypecodes[133];
> +
> +#define TIFF_GCSETYPE_OFFSET 4001
> +extern const char *const ff_tiff_gcsetypecodes[35];
> +
> +#define TIFF_GEODETICDATUM_OFFSET 6201
> +extern const char *const ff_tiff_geodeticdatumcodes[120];
> +
> +#define TIFF_GEODETICDATUME_OFFSET 6001
> +extern const char *const ff_tiff_geodeticdatumecodes[35];
> +
> +#define TIFF_ELLIPSOID_OFFSET 7001
> +extern const char *const ff_tiff_ellipsoidcodes[35];
> +
> +#define TIFF_PRIMEMERIDIAN_OFFSET 8901
> +extern const char *const ff_tiff_primemeridiancodes[11];
> +
> +extern const Key_name ff_tiff_proj_cs_type_codes[978];
> +
> +extern const Key_name ff_tiff_projection_codes[298];
> +
> +#define COORD_TRANS_OFFSET 7001
> +extern const char *const ff_tiff_coord_trans_codes[27];
> +
> +#endif
> diff --git a/libavcodec/tiffenc.c b/libavcodec/tiffenc.c
> index d486b3f..a50515e 100644
> --- a/libavcodec/tiffenc.c
> +++ b/libavcodec/tiffenc.c
> @@ -38,8 +38,8 @@
>  #define TIFF_MAX_ENTRY 32

  
>  /** 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).
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to