[FFmpeg-devel] [PATCH] Rudimentary support for id3v2 APIC tags.

Clément Bœsch ubitux at gmail.com
Tue Dec 27 19:30:26 CET 2011


On Tue, Dec 27, 2011 at 09:50:22AM -0800, Adrian Drzewiecki wrote:
> Extend dictionary to support binary data. Then use that to store ID3 APIC (image)
> tags.
> ---
>  libavformat/id3v2.c |  112 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/dict.c    |   73 ++++++++++++++++++++++++++-------
>  libavutil/dict.h    |   19 +++++++++
>  3 files changed, 189 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index 9ee3271..6122b5d 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -274,6 +274,115 @@ static void read_ttag(AVFormatContext *s, AVIOContext *pb, int taglen, const cha
>          av_dict_set(&s->metadata, key, dst, dict_flags);
>  }
>  
> +/*
> + * Parse an image tag.
> + * The image will be prefixed by the mime-type string (\0 terminated) and
> + * 32bit byte size of the image.
> + */
> +static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen, const char *tag)
> +{
> +    /* from http://id3.org/id3v2.4.0-frames #4.14 */
> +    const static char *key[] = {
> +        "other_picture",
> +        "32x32_icon",
> +        "other_icon",
> +        "front_cover",
> +        "back_cover",
> +        "leaflet_page",
> +        "media_picture",
> +        "lead_performer_picture",
> +        "artist_picture",
> +        "conductor_picture",
> +        "band_picture",
> +        "compose_picture",
> +        "lyricist_picture",
> +        "location_picture",
> +        "recording_picture",
> +        "performance_picture",
> +        "screen_capture",
> +        "a_bright_coloured_fish",
> +        "illustration",
> +        "band_logo",
> +        "studio_logo"
> +    };
> +    static int max_key = sizeof(key)/sizeof(key[0]);
> +    char *mime_type = NULL;
> +    unsigned char text_encoding;
> +    unsigned char picture_type;
> +    int i, len;
> +    void *data = NULL;
> +
> +    text_encoding = avio_r8(pb);
> +    taglen --;

nit: please drop the space here and below.

> +
> +    /* read mime type */
> +    i = len = 0;
> +    for (;;) {
> +        unsigned char c = avio_r8(pb);
> +        taglen --;
> +        if (i == len) {
> +            len = len ? (2 * len) : 8;
> +            mime_type = av_realloc_f(mime_type, len, 1);
> +            if (!mime_type) {
> +                av_log(s, AV_LOG_ERROR, "Alloc of mime type failed\n");
> +                return;
> +            }
> +        }
> +        mime_type[i++] = c;
> +        if (!c)
> +            break;
> +        if (!taglen) {
> +            av_log(s, AV_LOG_ERROR, "Couldn't find end of mime type\n");
> +            av_free(mime_type);
> +            return;

You should make a fail label with av_free(mime_type) at the end of the
function and use it.

> +        }
> +    }
> +    len = i;
> +
> +    if (!taglen) {
> +        av_log(s, AV_LOG_ERROR, "Truncated APIC tag.\n");
> +        av_free(mime_type);
> +        return;
> +    }
> +
> +    /* which tag */
> +    picture_type = avio_r8(pb);
> +    taglen --;
> +
> +    av_log(s, AV_LOG_INFO, "Found %s APIC\n", mime_type);
> +
> +    if (picture_type >= max_key || !key[picture_type]) {

drop max_key and use FF_ARRAY_ELEMS

> +        av_log(s, AV_LOG_INFO, "Unsupported picture type %x\n", picture_type);
> +        av_free(mime_type);
> +        return;
> +    }
> +
> +    av_log(s, AV_LOG_INFO, "APIC is %s\n", key[picture_type]);
> +
> +    /* skip over description */
> +    while (taglen && avio_r8(pb))
> +        taglen --;
> +
> +    if (!taglen) {
> +        av_log(s, AV_LOG_ERROR, "Truncated APIC tag.\n");
> +        av_free(mime_type);
> +        return;
> +    }
> +
> +    data = av_malloc(taglen);
> +    if (!data) {
> +        av_log(s, AV_LOG_ERROR, "Failed to allocate image buffer.\n");
> +        av_free(mime_type);
> +        return;
> +    }
> +
> +    avio_read(pb, data, taglen);
> +
> +    av_log(s, AV_LOG_INFO, "Read image buffer of %d bytes\n", taglen);
> +    av_dict_set_data(&s->metadata, key[picture_type], mime_type, data, taglen,
> +		     AV_DICT_DONT_STRDUP_VAL | AV_DICT_DONT_COPY_DATA);

Tabs are not allowed in FFmpeg.

> +}
> +
>  /**
>   * Parse GEOB tag into a ID3v2ExtraMetaGEOB struct.
>   */
> @@ -597,6 +706,9 @@ static void ff_id3v2_parse(AVFormatContext *s, int len, uint8_t version, uint8_t
>                  /* parse special meta tag */
>                  extra_func->read(s, pbx, tlen, tag, extra_meta);
>          }
> +        else if (!strcmp(tag, "APIC"))
> +          /* an image! */
> +          read_apic(s, pbx, tlen, tag);

4 spaces align

>          else if (!tag[0]) {
>              if (tag[1])
>                  av_log(s, AV_LOG_WARNING, "invalid frame id, assuming padding");
> diff --git a/libavutil/dict.c b/libavutil/dict.c
> index 6177ddd..55b7c43 100644
> --- a/libavutil/dict.c
> +++ b/libavutil/dict.c
> @@ -49,12 +49,21 @@ av_dict_get(AVDictionary *m, const char *key, const AVDictionaryEntry *prev, int
>  
>  int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags)
>  {
> +    return av_dict_set_data(pm, key, value, NULL, 0, flags);
> +}
> +
> +int av_dict_set_data(AVDictionary **pm, const char *key, const char *value, 
> +        const void *data, int size, int flags)

Vertical align the "const void *data" with "AVDict..."

> +{
>      AVDictionary      *m = *pm;
>      AVDictionaryEntry *tag = av_dict_get(m, key, NULL, flags);
>      char *oldval = NULL;
>  
> -    if(!m)
> +    if(!m) {
>          m = *pm = av_mallocz(sizeof(*m));
> +        if (!m)
> +            return AVERROR(ENOMEM);
> +    }
>  

This looks like an unrelated change.

>      if(tag) {
>          if (flags & AV_DICT_DONT_OVERWRITE)
> @@ -63,38 +72,71 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags
>              oldval = tag->value;
>          else
>              av_free(tag->value);
> +        av_free(tag->data);
>          av_free(tag->key);
>          *tag = m->elems[--m->count];
>      } else {
> -        AVDictionaryEntry *tmp = av_realloc(m->elems, (m->count+1) * sizeof(*m->elems));
> -        if(tmp) {
> -            m->elems = tmp;
> -        } else
> +        tag = av_realloc(m->elems, (m->count+1) * sizeof(*m->elems));
> +        if (!tag)
>              return AVERROR(ENOMEM);
> +        m->elems = tag;
>      }
> +
>      if (value) {
> -        if (flags & AV_DICT_DONT_STRDUP_KEY) {
> -            m->elems[m->count].key   = (char*)(intptr_t)key;
> -        } else
> -        m->elems[m->count].key  = av_strdup(key  );
> +        tag = &m->elems[m->count];
> +        memset(tag, 0, sizeof *tag);
> +
> +        if (flags & AV_DICT_DONT_STRDUP_KEY)
> +            tag->key = (char*)(intptr_t)key;
> +        else {
> +            tag->key = av_strdup(key);
> +            if (!tag->key)
> +                goto nomem;
> +        }
> +
>          if (flags & AV_DICT_DONT_STRDUP_VAL) {
> -            m->elems[m->count].value = (char*)(intptr_t)value;
> +            tag->value = (char*)(intptr_t)value;
>          } else if (oldval && flags & AV_DICT_APPEND) {
>              int len = strlen(oldval) + strlen(value) + 1;
>              if (!(oldval = av_realloc(oldval, len)))
> -                return AVERROR(ENOMEM);
> +                goto nomem;
>              av_strlcat(oldval, value, len);
> -            m->elems[m->count].value = oldval;
> -        } else
> -            m->elems[m->count].value = av_strdup(value);
> +            tag->value = oldval;
> +            oldval = NULL;
> +        } else {
> +            tag->value = av_strdup(value);
> +            if (!tag->value)
> +                goto nomem;
> +        }
> +         

trailing whitespaces here and a much more later

> +        if (data) {
> +            if (flags & AV_DICT_DONT_COPY_DATA)
> +                tag->data = (void *)(intptr_t)data;
> +            else {
> +                tag->data = av_malloc(size);
> +                if (!tag->data)
> +                    goto nomem;
> +                memcpy(tag->data, data, size);
> +            }
> +        }
> +        
> +        tag->size = size;
>          m->count++;
>      }
> +

not related

>      if (!m->count) {
>          av_free(m->elems);
>          av_freep(pm);
>      }
>  
>      return 0;
> +nomem:
> +    if (!(flags & AV_DICT_DONT_STRDUP_KEY))
> +        av_free(tag->key);
> +    if (!(flags & AV_DICT_DONT_STRDUP_VAL))
> +        av_free(tag->value);
> +    av_free(oldval);
> +    return AVERROR(ENOMEM);
>  }
>  
>  void av_dict_free(AVDictionary **pm)
> @@ -105,6 +147,7 @@ void av_dict_free(AVDictionary **pm)
>          while(m->count--) {
>              av_free(m->elems[m->count].key);
>              av_free(m->elems[m->count].value);
> +            av_free(m->elems[m->count].data);
>          }
>          av_free(m->elems);
>      }
> @@ -116,5 +159,5 @@ void av_dict_copy(AVDictionary **dst, AVDictionary *src, int flags)
>      AVDictionaryEntry *t = NULL;
>  
>      while ((t = av_dict_get(src, "", t, AV_DICT_IGNORE_SUFFIX)))
> -        av_dict_set(dst, t->key, t->value, flags);
> +        av_dict_set_data(dst, t->key, t->value, t->data, t->size, flags);
>  }
> diff --git a/libavutil/dict.h b/libavutil/dict.h
> index 2adf28c..6963594 100644
> --- a/libavutil/dict.h
> +++ b/libavutil/dict.h
> @@ -74,9 +74,13 @@
>  #define AV_DICT_APPEND         32   /**< If the entry already exists, append to it.  Note that no
>                                        delimiter is added, the strings are simply concatenated. */
>  
> +#define AV_DICT_DONT_COPY_DATA 64   /**< Do not copy entry data */
> +
>  typedef struct {
>      char *key;
>      char *value;
> +    void *data;
> +    int  size;
>  } AVDictionaryEntry;
>  
>  typedef struct AVDictionary AVDictionary;
> @@ -105,6 +109,21 @@ av_dict_get(AVDictionary *m, const char *key, const AVDictionaryEntry *prev, int
>  int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags);
>  
>  /**
> + * Set the given entry in *pm, overwriting an existing entry.
> + *
> + * @param pm pointer to a pointer to a dictionary struct. If *pm is NULL
> + * a dictionary struct is allocated and put in *pm.
> + * @param key entry key to add to *pm (will be av_strduped depending on flags)
> + * @param value entry value to add to *pm (will be av_strduped depending on flags).
> + *        Passing a NULL value will cause an existing tag to be deleted.
> + * @param data additional value to associate with key
> + * @param size size of additional data
> + * @return >= 0 on success otherwise an error code <0
> + */
> +int av_dict_set_data(AVDictionary **pm, const char *key, const char *value, 
> +        const void *data, int size, int flags);
> +

Vertical align here too, remove the trailing whitespace, and you should
also bump lavu minor (version.h) because of the add (and eventually
document it to doc/APIChanges).

Ideally you would split the introduction of av_dict_set_data from the ID3
patch.

Also, can we make use of AV_DICT_APPEND in case of binary entries?

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111227/ba2f9b83/attachment.asc>


More information about the ffmpeg-devel mailing list