[FFmpeg-devel] [PATCH] matroskaenc: write tags.

[Aurelien Jacobs]

On Mon, Oct 04, 2010 at 09:35:37AM +0200, Anton Khirnov wrote:
> Apparrently attempt to convert the keys to uppercase during metadata conversion
> step isn't going anywhere, so i've moved it directly to the muxer
> instead.
> 
> Please review.
> 
> [...]
> 
> +static void mkv_write_simpletag(ByteIOContext *pb, AVMetadataTag *t)
> +{
> +    uint8_t *key = av_strdup(t->key);
> +    uint8_t *p   = key;
> +    const uint8_t *lang = NULL;
> +    ebml_master tag;
> +
> +    while ((p = strchr(p, '-'))) {

Why not using strrchr() instead ? This should avoid the loop...

> +        if ((lang = av_convert_lang_to(p + 1, AV_LANG_ISO639_2_BIBL))) {
> +            *p = 0;
> +            break;
> +        }
> +        p++;
> +    }
> +
> +    p = key;
> +    while (*p) {
> +        if (*p == ' ')
> +            *p = '_';
> +        else if (*p >= 'a' && *p <= 'z')
> +            *p = toupper(*p);

I'm not sure about all the implications about using toupper (especially
depending on the $LANG and friends)... It may be safe as you already
check the range of the source character, but I wouldn't bet on it.
Maybe this would be better:
    *p -= 'a' - 'A';

> +        p++;
> +    }
> +
> +    tag = start_ebml_master(pb, MATROSKA_ID_SIMPLETAG, 0);
> +    put_ebml_string(pb, MATROSKA_ID_TAGNAME,   key);
                                              ^^^^^
Strange spacing.

> +    if (lang)
> +        put_ebml_string(pb, MATROSKA_ID_TAGLANG, lang);
> +    put_ebml_string(pb, MATROSKA_ID_TAGSTRING, t->value);
> +    end_ebml_master(pb, tag);
> +
> +    av_freep(&key);
> +}
> +
> +static int mkv_write_tag(AVFormatContext *s, AVMetadata *m, unsigned int elementid,

Long line.

> + [...]
> +
> +static int mkv_write_tags(AVFormatContext *s)
> +{
> +    ebml_master        tags = {0};

Strange spacing.

> + [...]

Overall, the code contains a bunch of slightly long lines, but this file
is already full of such long lines, so I guess it is OK.

Except those remarks, this patch looks pretty good to me.

Aurel