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

[Anton Khirnov]

On Mon, Oct 04, 2010 at 10:43:14PM +0200, Aurelien Jacobs wrote:
> 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...
right, fixed
> 
> > +        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';
yeah, done. as Reimar said, it might not always be right, but at least i
won't get any weird non-utf8 characters.
> 
> > +        p++;
> > +    }
> > +
> > +    tag = start_ebml_master(pb, MATROSKA_ID_SIMPLETAG, 0);
> > +    put_ebml_string(pb, MATROSKA_ID_TAGNAME,   key);
>                                               ^^^^^
> Strange spacing.
> 
fixed
> > +    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.
that's not even the longest line in the patch ;) neither this file, nor
ffmpeg in general are strictly following the no-lines-over-80-chars
rule, so i wouldn't bother with it. of course i will fix it if you
really insist.
> 
> > + [...]
> > +
> > +static int mkv_write_tags(AVFormatContext *s)
> > +{
> > +    ebml_master        tags = {0};
> 
> Strange spacing.
> 
fixed

-- 
Anton Khirnov
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-matroskaenc-write-tags.patch
Type: text/x-diff
Size: 4507 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101005/45ccd2af/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101005/45ccd2af/attachment.pgp>