[FFmpeg-devel] LRC (music lyrics) muxer and demuxer

[Peter Ross]

On Sun, Dec 26, 2010 at 10:00:59PM +0100, Aurelien Jacobs wrote:
> On Sun, Dec 26, 2010 at 11:45:50PM +1100, Peter Ross wrote:
> > Typical sample: http://lrc.awardspace.us/lrc/Evanescence-Missing.lrc
> 
> First some general comments.
> 
> Using CODEC_ID_TEXT don't seem appropriate.
> We need to be able to decode the "Simple format extended" (gender
> specifier) and the "Enhanced format" (Word Time Tag).
> See : http://en.wikipedia.org/wiki/LRC_%28file_format%29
> So for this to work, we need a specific CODEC_ID_LRC (and we will need
> to implement an encoder and a decoder, but that's another story).

Yep, that stuff definately belongs in the decoder.

> It seems you also need to support multiple timestamps on a single line
> (allows to avoid duplicating the same text several times).
> See : http://www.mobile-mir.com/en/HowToLRC.php
> And for a real world sample file, see:
> http://www.lyrdb.com/karaoke/18374.htm

Okay.

> Also, I wonder how we will generate AVSubtitle.end_display_time in the
> decoder ? In LRC, the start time of the next frame is the end time of
> current frame. The decoder could wait for the next frame before
> returning current frame, adding a 1 frame delay, but with subtitles, the
> time between two frames can be really long and the video would continue
> to decode while the subtitle is waiting for next frame to appear to know
> the end time of the frame which is supposed to be displayed now. So I
> guess this wouldn't really work.
> Any idea about how we could handle this ?

If the demuxer parses the entire file at file_open() time, then it can
calculate the duration and pass that onto the decoder via AVPacket->duration.

Also some files use an blank lyric line to force the previous line to 
disappear from the display.

> Now about the code itself.
> 
> > [...]
> > +const AVMetadataConv ff_lrc_metadata_conv[] = {
> > +    { "al", "album"      },
> > +    { "ar", "artist"     },
> > +    { "au", "composer"   },
> > +    { "by", "encoded_by" },
> > +    { "la", "language"   },
> > +    { "re", "encoder"    },
> > +    { "ti", "title"      },
> > +    { 0 },
> > +};
> 
> You may want to add:
>  +    { "ve", "encoder_version" },

Ok.

> > +static int probe(AVProbeData *p)
> > +{
> > +    const AVMetadataConv * c;
> > +    if (p->buf[0] != '[')
> > +        return 0;
> > +    for (c = ff_lrc_metadata_conv; c->native; c++) {
> > +        int len = strlen(c->native);
> > +        if (p->buf_size >= len + 2 && !memcmp(p->buf + 1, c->native, len) &&
> > +            p->buf[len + 1]==':')
> > +            return AVPROBE_SCORE_MAX/2 + 1;
> > +    }
> > +    return 0;
> > +}
> 
> This probe function is a bit week. Maybe you could test that the line
> correctly ends with a ']'. And you could also check that the second line
> is also correct.
> Also this autodetect fonction won't work for files with no metadata.

Agree.

> > +static int read_header(AVFormatContext *s, AVFormatParameters *ap)
> > +{
> > +    AVStream *st = av_new_stream(s, 0);
> > +    if (!st)
> > +        return -1;
> > +    st->codec->codec_type = AVMEDIA_TYPE_SUBTITLE;
> > +    st->codec->codec_id   = CODEC_ID_TEXT;
> > +    st->start_time        = 0;
> > +    av_set_pts_info(st, 64, 1, 100);
> 
> > +    ff_metadata_conv_ctx(s, ff_lrc_metadata_conv, NULL);
> 
> What is this ff_metadata_conv_ctx() good for ?

That tells ffmpeg to automatically translate betwen the LRC metadata keys
and the generic ffmpeg keys. e.g. 'ti' == 'title'. Somebody feel free
to correct me on this.

> > [...]
> > +            split = strchr(start, ':');
> > +            if (!split)
> > +                 continue;
> > +            end = strchr(split, ']');
> > +            if (!end)
> > +                 continue;
> 
> Could be simplified this way (at least I find it simpler):
> 
> +            if (!(split = strchr(start, ':')) ||
> +                !(end   = strchr(split, ']')))
> +                 continue;
> 
> > [...]
> > +            for (c = ff_lrc_metadata_conv; c->native; c++) {
> > +                int len = split - start;
> > +                if (strlen(c->native) == len && !memcmp(start, c->native, len))
> > +                    av_metadata_set2(&s->metadata, c->native, split + 1, 0);
> > +            }
> 
> I might simplify your code if you use ff_metadata_conv() ?
> 
> > [...]
> > +
> > +    end = strchr(start, '\n');
> > +    av_new_packet(pkt, end ? end - start : strlen(start));
> 
> It seems you drop the final '\n' (but not the '\r'). Why ?
> Other subtitles demuxers are outputing \r\n as part of the packet.

Ok.

> > [...]
> > +AVInputFormat lrc_demuxer = {
> > +    .name           = "lrc",
> 
> > +    .long_name      = NULL_IF_CONFIG_SMALL("LRC"),
> 
> Not a very descriptive long name...

'LRC lyrics'? Suggestions welcome.

> > +static int write_header(AVFormatContext *s)
> > +{
> > +    AVMetadataTag *t = NULL;
> > +    if (s->nb_streams != 1)
> > +        return -1;
> 
> Here you should also check that codec_id is the one supported.
> 
> > [...]
> > +
> > +AVOutputFormat lrc_muxer = {
> > +    .name           = "lrc",
> > +    .long_name      = NULL_IF_CONFIG_SMALL("LRC"),
> > +    .extensions     = "lrc",
> > +    .subtitle_codec = CODEC_ID_TEXT,
> > +    .write_header   = write_header,
> > +    .write_packet   = write_packet,
> 
> > +    .flags          = AVFMT_GLOBALHEADER,
> 
> Is this really needed/useful/appropriate ?
> This flag supposedly mean that the muxer expect some extradata to be
> available, but I don't think it tells anything about metadata.

Correct. Thanks for the review.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- 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/20101229/9f07affe/attachment.pgp>