[FFmpeg-devel] [PATCH 2/2] Add MPlayer subtitles demuxer and decoder.

[Clément Bœsch]

On Sun, Dec 30, 2012 at 03:32:20PM +0100, Nicolas George wrote:
[...]
> > +/**
> > + * @file
> > + * MPlayer subtitles format demuxer
> > + */
> 
> Reference to syntax or excerpt, maybe?
> 

AFAIK, the only available reference is this documented sample:
http://www.mplayerhq.hu/DOCS/tech/mpsub.sub

...which is used as a FATE sample for one of the test:
sub/MPSub_capability_tester.sub

> > +
> > +#include "avformat.h"
> > +#include "internal.h"
> > +#include "subtitles.h"
> > +
> > +typedef struct {
> > +    FFDemuxSubtitlesQueue q;
> > +} MPSubContext;
> > +
> > +static int mpsub_probe(AVProbeData *p)
> > +{
> > +    const char *ptr     = p->buf;
> > +    const char *ptr_end = p->buf + p->buf_size;
> > +
> > +    while (ptr < ptr_end) {
> > +        int n;
> > +
> > +        if (!memcmp(ptr, "FORMAT=TIME", 11) ||
> 
> > +            sscanf(ptr, "FORMAT=%d", &n) == 1)
> > +            return AVPROBE_SCORE_MAX;
> > +        ptr += strcspn(ptr, "\n") + 1;
> 
> A bit lax, maybe?
> 

now AVPROBE_SCORE_MAX/2

> > +    }
> > +    return 0;
> > +}
> > +
> > +static int mpsub_read_header(AVFormatContext *s)
> > +{
> > +    MPSubContext *mpsub = s->priv_data;
> > +    AVStream *st;
> > +    AVBPrint buf;
> > +    AVRational pts_info = (AVRational){ 100, 1 }; // ts based by default
> > +    int res = 0;
> > +    float multiplier = 100.0;
> > +    float current_pts = 0;
> > +
> > +    av_bprint_init(&buf, 0, AV_BPRINT_SIZE_UNLIMITED);
> > +
> > +    while (!url_feof(s->pb)) {
> > +        char line[1024];
> > +        float start, duration;
> > +        int fps, len = ff_get_line(s->pb, line, sizeof(line));
> > +
> > +        if (!len)
> > +            break;
> > +
> > +        line[strcspn(line, "\r\n")] = 0;
> > +
> 
> > +        if (sscanf(line, "FORMAT=%d", &fps) == 1 && fps > 3 && fps < 100) {
> > +            /* frame based timing */
> 
> What happens if there are several FORMAT lines?
> 

undefined behaviour; it's supposed to appear at the top.

> > +            pts_info = av_d2q(fps, 100000);
> 
> fps is an integer: why use av_d2q instead of just coding { fps, 1 }?
> 

Lame nobrain copy paste from MicroDVD, fixed.

> > +            multiplier = 1.0;
> > +        } else if (sscanf(line, "%f %f", &start, &duration) == 2) {
> > +            AVPacket *sub;
> > +            const int64_t pos = avio_tell(s->pb);
> > +
> > +            ff_subtitles_read_chunk(s->pb, &buf);
> > +            if (buf.len) {
> > +                sub = ff_subtitles_queue_insert(&mpsub->q, buf.str, buf.len, 0);
> > +                if (!sub) {
> > +                    res = AVERROR(ENOMEM);
> > +                    goto end;
> > +                }
> > +                sub->pts = (int64_t)(current_pts + start*multiplier);
> > +                sub->duration = (int)(duration * multiplier);
> > +                current_pts += (start + duration) * multiplier;
> > +                sub->pos = pos;
> > +            }
> > +        }
> 
> No error report if garbage is found?
> 

There are several settings not supported which may trigger that error, so
I'd better not.

[...]

Thanks for the review, pushed.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121230/1619ffc4/attachment.asc>