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

[Reimar Döffinger]

On Sun, Oct 31, 2010 at 06:11:59PM +0100, Anton Khirnov wrote:
> +static int is_number(const char *p)
> +{
> +    while (*p)
> +        if (!isdigit(*p++))

Is digit depends on locale, are you sure this correct in this context?

> +static void merge_date(AVMetadata **m)
> +{
> +    const char    *keys[]  = {"TYER", "TDAT", "TIME"};

Why not "static const"? gcc usually will convert it on its own,
but you risk it being pedantic and copying the data onto the stack
pointlessly.
Also storing pointers instead of the strings directly cost space and
performance.

> +    AVMetadataTag *tags[3] = {0};

I think it is preferable to use NULL instead of 0 for pointers.
Also you could just write all elements in this case.

> +    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 : "";
....

> +    for (i = 0; i < sizeof(keys); i++)

Here and above: certainly not, FF_ARRAY_ELEMS or what it is called could
be used.