[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