[FFmpeg-devel] [PATCH] Add Apple HTTP Live Streaming protocol handler

[Ronald S. Bultje]

Hi,

On Tue, Jul 27, 2010 at 5:45 AM, Martin Storsj? <martin at martin.st> wrote:
[..]
> +struct segment {
> +    int duration;
> +    char *url;
> +};
> +
> +struct variant {
> +    int bandwidth;
> +    char *url;
> +    ByteIOContext *pb;
> +    AVFormatContext *ctx;
> +    AVPacket pkt;
> +    int stream_offset;
> +
> +    int start_seq_no;
> +    int n_segments;
> +    struct segment **segments;
> +    int needed;
> +};

Each of these (and below) could use at least a few doxy words to
explain what they are. What is a segment? What is a variant?

Is it possible to make url in both segment and vriant fixed-length, so
that we don't have to alloc so may individual elements? Not critical,
but would be nice.

> +typedef struct AppleHTTPContext {
> +    char playlisturl[2048];

Unused (and kinda big).

> +    int64_t last_load_time;

You're assigning this with av_gettime(), which seems wrong if
clockrate server/client are off. I think it's better if you use PTS
here instead of clocktime.

> +static int read_chomp_line(ByteIOContext *s, char *buf, int maxlen)
> +{
> +    int len = ff_get_line(s, buf, maxlen);
> +    if (len > 0 && (buf[len-1] == '\r' || buf[len-1] == '\n'))
> +        buf[--len] = '\0';
> +    if (len > 0 && (buf[len-1] == '\r' || buf[len-1] == '\n'))
> +        buf[--len] = '\0';
> +    return len;
> +}

This could be simplified (if ...buf[len-1]=='\n' { if
(buf[len-2]=='n') .. else .. }), but that's not too critical.

You can presumably also chop off all space-like characters, assuming
that's valid.

> +static void reset_packet(AVPacket *pkt)
> +{
> +    av_init_packet(pkt);
> +    pkt->data = NULL;
> +}

This seems overused. When is the data=NULL necessary, and what does it
mean? What happens if we don't?

> +static struct variant *new_variant(AppleHTTPContext *c, int bandwidth,
> +                                   const char *url)
> +{
> +    struct variant *var = av_mallocz(sizeof(struct variant));
> +    if (!var)
> +        return NULL;
> +    reset_packet(&var->pkt);

> +    var->bandwidth = bandwidth;
> +    var->url = av_strdup(url);

Vertical align (also in other places). I'm wondering if we can just
av_strlcpy() into a fixed size buffer here (as said above).

[..]
> +static int parse_playlist(AppleHTTPContext *c, const char *url,
> +                          struct variant *var)
> +{
> +    ByteIOContext *in;
[..]
> +    if ((ret = url_fopen(&in, url, URL_RDONLY)) < 0)
> +        return ret;

This should already be open in s->pb. Why do you want to reopen it?

> +static int applehttp_read_header(AVFormatContext *s, AVFormatParameters *ap)
[..]
> +    if (c->n_variants > 1 || c->variants[0]->n_segments == 0) {
> +        char urlbuf[2048];
> +        for (i = 0; i < c->n_variants; i++) {
> +            make_absolute_url(urlbuf, sizeof(urlbuf), s->filename,
> +                              c->variants[i]->url);
> +            av_freep(&c->variants[i]->url);
> +            c->variants[i]->url = av_strdup(urlbuf);
> +            if ((ret = parse_playlist(c, c->variants[i]->url,
> +                                      c->variants[i])) < 0)
> +                goto fail;
> +        }
> +    }

The idea is that this should be transparent to the user. If the file
contains a playlist, it should be handled as just any other file, and
that should _work_, just like that. If it doesn't, it's a bug and
should be fixed.

Therefore, this code shouldn't be necessary.

> +    if (c->variants[0]->n_segments == 0) {
> +        av_log(NULL, AV_LOG_WARNING, "Empty playlist\n");
> +        ret = AVERROR_EOF;
> +        goto fail;
> +    }

This would then change to if (contained-data-length == 0) ... fail.

> +    if (c->finished) {
> +        int duration = 0;
> +        for (i = 0; i < c->variants[0]->n_segments; i++)
> +            duration += c->variants[0]->segments[i]->duration;
> +        s->duration = duration * AV_TIME_BASE;
> +    }

Why if (finished)? Isn't it an error if it's not? (And otherwise,
what's duration?)

> +static int applehttp_read_packet(AVFormatContext *s, AVPacket *pkt)

This function could use some documentation / comments.

> +    for (i = 0; i < c->n_variants; i++)
> +        c->variants[i]->needed = 0;
> +    for (i = 0; i < s->nb_streams; i++) {
> +        AVStream *st = s->streams[i];
> +        struct variant *var = c->variants[s->streams[i]->id];
> +        if (st->discard < AVDISCARD_ALL) {
> +            var->needed = 1;
> +            needed++;
> +        }
> +        var->ctx->streams[i - var->stream_offset]->discard = st->discard;
> +    }

Why are these context variables. Why not a array of size MAXSTREAMS or
so? They don't need to be saved across runs.

> +    if (!c->finished) {
> +        int64_t now = av_gettime();
> +        if (now - c->last_load_time >= c->target_duration*1000000) {
> +            c->max_start_seq = 0;
> +            c->min_end_seq   = INT_MAX;
> +            for (i = 0; i < c->n_variants; i++) {
> +                struct variant *var = c->variants[i];
> +                if (var->needed) {
> +                    if ((ret = parse_playlist(c, var->url, var)) < 0)
> +                        return ret;
> +                    c->max_start_seq = FFMAX(c->max_start_seq,
> +                                             var->start_seq_no);
> +                    c->min_end_seq   = FFMIN(c->min_end_seq,
> +                                             var->start_seq_no + var->n_segments);


Eh... So this is a playlist-in-a-playlist? I'm again confused, this
should be transparent. And shouldn't this be a media file at this
point? again, some documentation / comments would help me understand
what the code is doing.

> +    if (c->cur_seq_no < c->max_start_seq) {
> +        av_log(NULL, AV_LOG_WARNING,
> +               "skipping %d segments ahead, expired from playlists\n",
> +               c->max_start_seq - c->cur_seq_no);
> +        c->cur_seq_no = c->max_start_seq;
> +    }
> +    if (c->cur_seq_no >= c->min_end_seq) {
> +        if (c->finished)
> +            return AVERROR_EOF;
> +        while (av_gettime() - c->last_load_time < c->target_duration*1000000) {
> +            if (url_interrupt_cb())
> +                return AVERROR(EINTR);
> +            usleep(100*1000);
> +        }
> +        goto retry;
> +    }
> +    goto start;
> +}

I personally object to the use of av_gettime() here...

(Skipped seeking code for this cycle.)

> +    .extensions = "m3u8",

?? Really?

Is a probe function possible based on the expected EXT-attributes in
it that are relatively Apple-specific?

Ronald