[FFmpeg-soc] [soc]: r4321 - in concat/libavformat: . m3u.c playlist.c playlist.h

[Baptiste Coudurier]

Hi,

Overall comment,

Placement of braces in FFmpeg is:
while () {
}

With or without the whitespaces, as you wish.

gkovacs wrote:
> +
> +/*
> + * Based on AU muxer and demuxer in au.c
> + */
> +

You left a bit too much stuff from it though :>

> [...]
>
> +static int m3u_write_trailer(AVFormatContext *s)
> +{
> +    ByteIOContext *pb = s->pb;
> +    int64_t file_size;
> +
> +    if (!url_is_streamed(s->pb)) {
> +
> +        /* update file size */
> +        file_size = url_ftell(pb);
> +        url_fseek(pb, 8, SEEK_SET);
> +        put_be32(pb, (uint32_t)(file_size - 24));
> +        url_fseek(pb, file_size, SEEK_SET);
> +
> +        put_flush_packet(pb);
> +    }
> +

Forget about the muxer for now, you can remove it.

> [...]
>
> +static int compare_bufs(unsigned char *buf, unsigned char *rbuf)
> +{
> +    while (*rbuf != 0)
> +    {
> +        if (*rbuf != *buf)
> +            return 0;
> +        ++buf;
> +        ++rbuf;
> +    }
> +    return 1;

Humm, strcmp ?

> +static int m3u_probe(AVProbeData *p)
> +{
> +    if (p->buf_size >= 7 && p->buf != 0)
> +    {
> +        if (compare_bufs(p->buf, "#EXTM3U"))
> +            return AVPROBE_SCORE_MAX;
> +        else
> +            return 0;
> +    }
> +    if (check_file_extn(p->filename, "m3u"))
> +        return AVPROBE_SCORE_MAX/2;
> +    else
> +        return 0;
> +}

use match_ext instead of check_file_extn
p->buf must be at least AVPROBE_PADDING_SIZE (32)

> +
> +static int m3u_list_files(unsigned char *buffer, int buffer_size, unsigned char **file_list, unsigned int *lfx_ptr)
> +{
> +    unsigned int i;
> +    unsigned int lfx = 0;
> +    unsigned int fx = 0;
> +    unsigned char hashed_out = 0;
> +    unsigned char fldata[262144];
> +    memset(fldata, 0, 262144);

This is big stack allocation I think. You can use realloc.

> +    memset(file_list, 0, 512 * sizeof(unsigned char*));
> +    for (i = 0; i < 512; ++i)
> +    {
> +        file_list[i] = fldata+i*512;
> +    }

Having a 512 limitation seems bad.

> +    for (i = 0; i < buffer_size; ++i)
> +    {
> +        if (buffer[i] == 0)
> +            break;
> +        if (buffer[i] == '#')
> +        {
> +            hashed_out = 1;
> +            continue;
> +        }
> +        if (buffer[i] == '\n')
> +        {
> +            hashed_out = 0;
> +            if (fx != 0)
> +            {
> +                fx = 0;
> +                ++lfx;
> +            }
> +            continue;
> +        }
> +        if (hashed_out)
> +            continue;
> +        file_list[lfx][fx] = buffer[i];
> +        ++fx;
> +    }
> +    *lfx_ptr = lfx;
> +    return 0;
> +}

I think you can simplify using url_fgets.

> +/* m3u input */
> +static int m3u_read_header(AVFormatContext *s,
> +                          AVFormatParameters *ap)
> +{
> +    unsigned char *flist[512];
> +    int flist_len;
> +    ByteIOContext *pb;
> +    PlaylistD *playld;
> +    pb = s->pb;
> +    m3u_list_files(pb->buffer, pb->buffer_size, flist, &flist_len);
> +    playld = av_make_playlistd(flist, flist_len);
> +    s->priv_data = playld;
> +    playlist_populate_context(playld, s);
> +    return 0;
> +}

Hummmm, you must not access pb->buffer directly.
I think it might be better to build a list of PlayElem directly.

Also I don't think user will want to open all files at the beginning,
it could be slow with network and file might not exist yet (being
downloaded).

> [...]
>
> +int av_open_input_playelem(PlayElem *pe)
> +{
> +    return av_open_input_file(&(pe->ic), pe->filename, pe->fmt, pe->buf_size, pe->ap);

Useless function wrapper.

> [...]
>
> +// based on decode_thread() in ffplay.c
> +int av_alloc_playelem(unsigned char *filename, PlayElem *pe)
> +{
> +//    AVProbeData *pd;
> +    AVFormatContext *ic;
> +    AVFormatParameters *ap;
> +//    pd = av_malloc(sizeof(AVProbeData));
> +//    ic = av_malloc(sizeof(AVFormatContext));
> +    ap = av_malloc(sizeof(AVFormatParameters));
> +    memset(ap, 0, sizeof(AVFormatParameters));
> +    ap->width = 0;
> +    ap->height = 0;
> +    ap->time_base = (AVRational){1, 25};
> +    ap->pix_fmt = 0;
> +//    pd->filename = filename;
> +//    pd->buf = NULL;
> +//    pd->buf_size = 0;
> +    pe->ic = ic;
> +    pe->filename = filename;
> +    pe->fmt = 0;
> +    pe->buf_size = 0;
> +    pe->ap = ap;
> +//    pe->fmt = pe->ic->iformat;
> +    return 0;
> +}

Please clean what's not necessary, we need a clean implementation,
and forget about AVFormatParameters for now.

> +PlayElem* av_make_playelem(unsigned char *filename)
> +{
> +    int err;
> +    PlayElem *pe = av_malloc(sizeof(PlayElem));
> +    err = av_alloc_playelem(filename, pe);
> +    if (err < 0)
> +        print_error("during-av_alloc_playelem", err);
> +    err = av_open_input_playelem(pe);
> +    if (err < 0)
> +        print_error("during-open_input_playelem", err);
> +    pe->fmt = pe->ic->iformat;
> +    if (!pe->fmt)
> +    {
> +        fprintf(stderr, "pefmt not set\n");
> +        fflush(stderr);

Why fflush ?

> +    }
> +    err = av_find_stream_info(pe->ic);
> +    if (err < 0)
> +    {
> +        fprintf(stderr, "failed codec probe av_find_stream_info");
> +        fflush(stderr);

Missing \n ?

> +    }
> +    if(pe->ic->pb)
> +    {
> +        pe->ic->pb->eof_reached = 0;
> +    }
> +    else
> +    {
> +        fprintf(stderr, "failed pe ic pb not set");
> +        fflush(stderr);
> +    }

Why do you need to reset eof_reached ?

Overall comment: do you feel confident everything will fit in a demuxer ?

I think some API calls will be useful like play 5th element in the
playlist, play next element, play prev, shuffle, etc...

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org