[FFmpeg-devel] [PATCH 2/4] id3v2: merge TYER/TDAT/TIME to date tag

[Aurelien Jacobs]

On Tue, Nov 02, 2010 at 10:16:31PM +0100, Anton Khirnov wrote:
> On Mon, Nov 01, 2010 at 07:54:00AM +0100, Reimar D?ffinger wrote:
> > On Sun, Oct 31, 2010 at 10:38:11PM +0100, Anton Khirnov wrote:
> > > On Sun, Oct 31, 2010 at 06:39:58PM +0100, Reimar D?ffinger wrote:
> > > > > +    for (i = 0; i < sizeof(keys); i++) {
> > > > > +        AVMetadataTag *t = av_metadata_get(*m, keys[i], NULL, AV_METADATA_MATCH_CASE);
> > > > > +        if (!t || strlen(t->value) != 4 || !is_number(t->value))
> > > > > +            break;
> > > > > +        tags[i] = t;
> > > > > +    }
> > > > > +
> > > > > +    if      (tags[2]) len = sizeof("YYYY-MM-DD HH:MM");
> > > > > +    else if (tags[1]) len = sizeof("YYYY-MM-DD");
> > > > > +    else if (tags[0]) len = sizeof("YYYY");
> > > > > +    else return;
> > > > > +
> > > > > +    if (!(date = av_malloc(len)))
> > > > > +        return;
> > > > > +    snprintf(date, len, "%.4s-%.2s-%.2s %.2s:%.2s",
> > > > > +             tags[0] ? tags[0]->value     : "",         // year
> > > > > +             tags[1] ? tags[1]->value + 2 : "",         // month
> > > > > +             tags[1] ? tags[1]->value     : "",         // day
> > > > > +             tags[2] ? tags[2]->value     : "",         // hour
> > > > > +             tags[2] ? tags[2]->value + 2 : "");        // minute
> > > > 
> > > > I suspect this could profit hugely from some restructuring, but at the very
> > > > least I'd suggest adding some variables and doing
> > > > year = tags[0] ? tags[0]->value : "";
> > > > ....
> > > > 
> > > i fail to see what that would accomplish other than making it longer
> > 
> > Making it a lot easier to understand what it does?
> > In particular, get rid of the comments that are likely to be wrong
> > after the next code change anyway?
> > It might also be possible to combine them with the if (tags[...])
> > checks that are already there anyway.
> ok, done
> 

> From bb82dc829c0a89d47f52790a7de277c928fa6488 Mon Sep 17 00:00:00 2001
> From: Anton Khirnov <anton at khirnov.net>
> Date: Sun, 10 Oct 2010 09:21:58 +0200
> Subject: [PATCH] id3v2: merge TYER/TDAT/TIME to date tag
> 
> ---
>  libavformat/id3v2.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index cc3697a..e92d624 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -162,6 +162,56 @@ static void read_ttag(AVFormatContext *s, ByteIOContext *pb, int taglen, const c
>          av_metadata_set2(&s->metadata, key, val, 0);
>  }
>  
> +static int is_number(const char *p)
> +{
> +    while (*p) {
> +        if (*p < '0' || *p > '9')
> +            return 0;
> +        p++;
> +    }
> +    return 1;
> +}
> +
> +static void merge_date(AVMetadata **m)
> +{
> +    static const char keys[][5] = {"TYER", "TDAT", "TIME"};
> +    AVMetadataTag    *tags[]    = { NULL ,  NULL ,  NULL };
> +    char *date, *year, month, day, hour, minute;
> +    int i, len = 0;
> +
> +    for (i = 0; i < FF_ARRAY_ELEMS(keys); i++) {
> +        AVMetadataTag *t = av_metadata_get(*m, keys[i], NULL, AV_METADATA_MATCH_CASE);
> +        if (!t || strlen(t->value) != 4 || !is_number(t->value))
> +            break;
> +        tags[i] = t;
> +    }
> +
> +    if (tags[0]) {
> +        len   += sizeof("YYYY") - 1;
> +        year   = tags[0]->value;
> +    } else
> +        return;
> +    if (tags[1]) {
> +        len   += sizeof("-MM-DD") - 1;
> +        month  = tags[1]->value + 2;
> +        day    = tags[1]->value;
> +    } else
> +        month = day = "";
> +    if (tags[2]) {
> +        len   += sizeof(" HH:MM") - 1;
> +        hour   = tags[2]->value;
> +        minute = tags[2]->value + 2;
> +    } else
> +        hour = minute = "";

Computing len seems pretty useless... Just fix its value to 16 and be
done with it. The few allocated bytes that might not be used really
won't hurt.

> +    if (!(date = av_malloc(len + 1)))
> +        return;
> +    snprintf(date, len, "%.4s-%.2s-%.2s %.2s:%.2s", year, month, day, hour, minute);
> +    av_metadata_set2(m, "date", date, AV_METADATA_DONT_STRDUP_VAL);

Actually it would be even simpler to just use:
    char date[17];
and drop the av_malloc() and the AV_METADATA_DONT_STRDUP_VAL.

Aurel