[FFmpeg-devel] [PATCH] Playlist API

Michael Niedermayer michaelni
Sun Aug 23 05:15:25 CEST 2009


On Fri, Aug 21, 2009 at 01:59:58AM +0900, Geza Kovacs wrote:
> This updated patch should address most of your comments.
> 
> On Mon, Aug 17, 2009 at 5:13 AM, Michael Niedermayer<michaelni at gmx.at> wrote:
> >
> >> +{
> >> + ? ?int i;
> >> + ? ?int64_t total = 0;
> >> + ? ?for (i = 0; i < pe_curidx; ++i) {
> >> + ? ? ? ?total += durations[i];
> >> + ? ?}
> >> + ? ?return total;
> >> +}
> >> +
> >
> >> +int ff_playlist_stream_index_from_time(PlaylistContext *ctx,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int64_t pts,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int64_t *localpts)
> >> +{
> >> + ? ?int i;
> >> + ? ?int64_t total;
> >> + ? ?i = total = 0;
> >> + ? ?while (pts >= total) {
> >> + ? ? ? ?if (i >= ctx->pelist_size)
> >> + ? ? ? ? ? ?break;
> >> + ? ? ? ?total += ctx->durations[i++];
> >> + ? ?}
> >
> > maybe the AVIndexEntry code could be used for these things?
> >
> 
> I don't think so - I would only be using the timestamps field out of
> AVIndexEntry so it would be simpler and more elegant to just use a
> standard array rather than structs.
> 

> >
> >> + ? ?int64_t *durations; /**< Durations, in AV_TIME_BASE units, for each playlist item */
> >
> > why is AVFormatContext.duration not used here?
> >
> 
> Since an application might potentially want to close and free the
> older child AVFormatContext once it's no longer in use, this is a more
> flexible design as it only requires the current master and child
> AVFormatContext to be allocated.

Unless the user needs some other field, metadata, codec type, filename, ...
anyway i see your point, the space could be annoying with a large playlist


> 
> >
> >> + ? ?int *nb_streams_list; /**< List of the number of streams in each playlist item*/
> >
> > why is AVFormatContext.nb_streams not used here?
> >
> 
> Same reason as mentioned above.
> 
> >
> >> +} PlaylistContext;
> >> +
> >
> >


> >> +/** @fn AVFormatContext *ff_playlist_alloc_formatcontext(char *filename)
> >
> > redundant
> >
> >> + * ?@brief Allocates and opens file, codecs, and streams associated with filename.
> >> + * ?@param filename Null-terminated string of file to open.
> >> + * ?@return Returns an allocated AVFormatContext.
> >> + */
> >> +AVFormatContext *ff_playlist_alloc_formatcontext(char *filename);
> >
> 
> Sorry, I didn't understand what you meant by this. Could you please elaborate?

the doxy text "@fn AVFormatContext *ff_playlist_alloc_formatcontext(char *filename)"
is redundant, thats alraedy in the code


[...]
> diff --git a/libavformat/avplaylist.c b/libavformat/avplaylist.c
> new file mode 100644
> index 0000000..73c4f1e
> --- /dev/null
> +++ b/libavformat/avplaylist.c
> @@ -0,0 +1,269 @@
> +/*
> + * General components used by playlist formats
> + * Copyright (c) 2009 Geza Kovacs
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/** @file libavformat/avplaylist.c
> + *  @author Geza Kovacs ( gkovacs mit edu )
> + *
> + *  @brief General components used by playlist formats
> + *
> + *  @details These functions are used to initialize and manipulate playlists
> + *  (AVPlaylistContext) and their individual playlist elements (PlayElem), each

PlayElem occurs only on the docs not in the code


> + *  of which encapsulates its own AVFormatContext. This abstraction is used for
> + *  implementing file concatenation and support for playlist formats.
> + */
> +
> +#include "libavformat/avplaylist.h"
> +#include "riff.h"
> +#include "libavutil/avstring.h"
> +#include "internal.h"
> +#include "concat.h"
> +

> +AVFormatContext *av_playlist_alloc_formatcontext(char *filename)
> +{
> +    int err;
> +    AVFormatContext *ic = avformat_alloc_context();
> +    err = av_open_input_file(&(ic), filename, ic->iformat, 0, NULL);

useless ()


> +    if (err < 0)
> +        av_log(ic, AV_LOG_ERROR, "Error during av_open_input_file\n");
> +    err = av_find_stream_info(ic);
> +    if (err < 0)
> +        av_log(ic, AV_LOG_ERROR, "Could not find stream info\n");

i dont think this error handling is sufficient


> +    return ic;
> +}
> +
> +void av_playlist_populate_context(AVPlaylistContext *ctx, int pe_curidx)
> +{
> +    ctx->icl = av_realloc(ctx->icl, sizeof(*(ctx->icl)) * (pe_curidx+2));
> +    ctx->icl[pe_curidx+1] = NULL;
> +    ctx->icl[pe_curidx] = av_playlist_alloc_formatcontext(ctx->flist[pe_curidx]);
> +    ctx->nb_streams_list[pe_curidx] = ctx->icl[pe_curidx]->nb_streams;
> +}
> +
> +void av_playlist_set_streams(AVFormatContext *s)
> +{
> +    int i;
> +    int offset;

> +    AVFormatContext *ic;
> +    AVPlaylistContext *ctx = s->priv_data;
> +    ic = ctx->icl[ctx->pe_curidx];

init and declaration can be merged


> +    offset = av_playlist_streams_offset_from_playidx(ctx, ctx->pe_curidx);
> +    ic->iformat->read_header(ic, NULL);
> +    for (i = 0; i < ic->nb_streams; ++i) {
> +        s->streams[offset + i] = ic->streams[i];
> +        ic->streams[i]->index += offset;
> +        if (!ic->streams[i]->codec->codec) {
> +            AVCodec *codec = avcodec_find_decoder(ic->streams[i]->codec->codec_id);
> +            if (!codec) {
> +                av_log(ic->streams[i]->codec, AV_LOG_ERROR, "Decoder (codec id %d) not found for input stream #%d\n",
> +                       ic->streams[i]->codec->codec_id, ic->streams[i]->index);
> +                return;
> +             }
> +             if (avcodec_open(ic->streams[i]->codec, codec) < 0) {
> +                av_log(ic->streams[i]->codec, AV_LOG_ERROR, "Error while opening decoder for input stream #%d\n",
> +                       ic->streams[i]->index);
> +                return;
> +             }
> +        }
> +    }

something is wrong with the indention here


[...]
> +void av_playlist_split_encodedstring(const char *s,
> +                                     const char sep,
> +                                     char ***flist_ptr,
> +                                     int *len_ptr)
> +{
> +    char c, *ts, **flist;
> +    int i, len, buflen, *sepidx;
> +    sepidx = NULL;
> +    buflen = len = 0;
> +    sepidx = av_fast_realloc(sepidx, &buflen, ++len);
> +    sepidx[0] = 0;
> +    ts = s;
> +    while ((c = *ts++) != 0) {
> +        if (c == sep) {
> +            sepidx[len] = ts-s;

> +            sepidx = av_fast_realloc(sepidx, &buflen, ++len);

memleak


[...]
> +void av_playlist_relative_paths(char **flist,
> +                                int len,
> +                                const char *workingdir)
> +{
> +    int i;
> +    for (i = 0; i < len; ++i) { // determine if relative paths
> +        char *fullfpath;

> +        int wdslen = strlen(workingdir);
> +        int flslen = strlen(flist[i]);

these variabke names are also a little poor


> +        fullfpath = av_malloc(wdslen+flslen+2);
> +        av_strlcpy(fullfpath, workingdir, wdslen+1);
> +        fullfpath[wdslen] = '/';
> +        fullfpath[wdslen+1] = 0;
> +        av_strlcat(fullfpath, flist[i], wdslen+flslen+2);
> +        if (url_exist(fullfpath))
> +            flist[i] = fullfpath;
> +    }
> +}
> +

> +int64_t av_playlist_time_offset(const int64_t *durations, int pe_curidx)
> +{
> +    int i;
> +    int64_t total = 0;
> +    for (i = 0; i < pe_curidx; ++i) {
> +        total += durations[i];
> +    }
> +    return total;
> +}
> +
> +int av_playlist_stream_index_from_time(AVPlaylistContext *ctx,
> +                                       int64_t pts,
> +                                       int64_t *localpts)
> +{
> +    int i;
> +    int64_t total;
> +    i = total = 0;
> +    while (pts >= total) {
> +        if (i >= ctx->pelist_size)
> +            break;
> +        total += ctx->durations[i++];
> +    }
> +    if (localpts)
> +        *localpts = pts-(total-ctx->durations[i-1]);
> +    return i;
> +}

maybe storing the sum of the past durations would be more convenient?
it would avoid the first function


> +
> +int av_playlist_playidx_from_streamidx(AVPlaylistContext *ctx, int stream_index)
> +{
> +    int i, total;
> +    i = total = 0;
> +    while (stream_index >= total)
> +        total += ctx->nb_streams_list[i++];
> +    return i-1;
> +}

this function is unused and can thus be removed


> +
> +int av_playlist_localstidx_from_streamidx(AVPlaylistContext *ctx, int stream_index)
> +{
> +    int i, total;
> +    i = total = 0;
> +    while (stream_index >= total)
> +        total += ctx->nb_streams_list[i++];
> +    return stream_index - (total - ctx->nb_streams_list[i-1]);
> +}
> +

> +int av_playlist_streams_offset_from_playidx(AVPlaylistContext *ctx, int playidx)
> +{
> +    int i, total;
> +    i = total = 0;
> +    while (playidx > i)
> +        total += ctx->nb_streams_list[i++];
> +    return total;
> +}

isnt used outside libav*, thus doesnt need to be public

besides i think your API is somewhat more complex than needed
i would have expected one function to add a playlist entry (that when
its context is NULL would allocate it) and one that frees a playlist
entry. Is more really usefull?



> +
> diff --git a/libavformat/avplaylist.h b/libavformat/avplaylist.h
> new file mode 100644
> index 0000000..0c82437
> --- /dev/null
> +++ b/libavformat/avplaylist.h
> @@ -0,0 +1,174 @@
> +/*
> + * General components used by playlist formats
> + * Copyright (c) 2009 Geza Kovacs
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/** @file libavformat/playlist.h
> + *  @author Geza Kovacs ( gkovacs mit edu )
> + *
> + *  @brief General components used by playlist formats
> + *
> + *  @details These functions are used to initialize and manipulate playlists
> + *  (AVPlaylistContext) and their individual playlist elements (PlayElem), each
> + *  of which encapsulates its own AVFormatContext. This abstraction is used for
> + *  implementing file concatenation and support for playlist formats.
> + */
> +
> +#ifndef AVFORMAT_PLAYLIST_H
> +#define AVFORMAT_PLAYLIST_H
> +
> +#include <libgen.h>
> +#include "avformat.h"
> +
> +/** @struct AVPlaylistContext
> + *  @brief Represents the playlist and contains PlayElem for each playlist item.
> + */
> +typedef struct AVPlaylistContext {
> +    char **flist;          /**< List of file names for each playlist item */

> +    AVFormatContext **icl; /**< List of FormatContext for each playlist items */

icl is a poor name, it doesnt say anything to me, could as well be abc
at the least the commet should clarify what icl means if no better name is
found


> +    int pelist_size;       /**< Number of playlist elements stored in icl */
> +    int pe_curidx;         /**< Index of the AVFormatContext in icl that packets are being read from */
> +    int64_t *durations;    /**< Durations, in AV_TIME_BASE units, for each playlist item */
> +    int *nb_streams_list;  /**< List of the number of streams in each playlist item*/
> +} AVPlaylistContext;
> +

> +/** @fn AVFormatContext *av_playlist_alloc_formatcontext(char *filename)
> + *  @brief Allocates and opens file, codecs, and streams associated with filename.
> + *  @param filename Null-terminated string of file to open.
> + *  @return Returns an allocated AVFormatContext.
> + */
> +AVFormatContext *av_playlist_alloc_formatcontext(char *filename);

with that a reader likely doesnt know what the difference is to the existing
file open code


[...]
> +int ff_concatgen_read_packet(AVFormatContext *s,
> +                             AVPacket *pkt)
> +{
> +    int ret, i, stream_index;
> +    AVPlaylistContext *ctx;
> +    AVFormatContext *ic;
> +    char have_switched_streams = 0;
> +    ctx = s->priv_data;
> +    stream_index = 0;
> +    for (;;) {
> +        ic = ctx->icl[ctx->pe_curidx];
> +        av_init_packet(pkt);
> +        if (s->packet_buffer) {
> +            *pkt = s->packet_buffer->pkt;
> +            s->packet_buffer = s->packet_buffer->next;
> +            ret = 0;
> +        } else {
> +            ret = ic->iformat->read_packet(ic, pkt);
> +        }
> +        if (ret >= 0) {
> +            if (pkt) {
> +                stream_index = av_playlist_localstidx_from_streamidx(ctx, pkt->stream_index);
> +                pkt->stream_index = stream_index + av_playlist_streams_offset_from_playidx(ctx, ctx->pe_curidx);

> +                if (!ic->streams[stream_index]->codec->has_b_frames ||
> +                    ic->streams[stream_index]->codec->codec->id == CODEC_ID_MPEG1VIDEO) {
> +                    pkt->dts += av_rescale_q(av_playlist_time_offset(ctx->durations, ctx->pe_curidx),
> +                                             AV_TIME_BASE_Q,
> +                                             ic->streams[stream_index]->time_base);
> +                    pkt->pts = pkt->dts + 1;
> +                }

as i said previously, this code is incorrect, both pts and dts has to be
offset correctly there is no +1 relation between them

also av_playlist_time_offset() is too slow to be run on each packet


> +            }
> +            break;
> +        } else {
> +            if (!have_switched_streams &&
> +                ctx->pe_curidx < ctx->pelist_size - 1) {
> +            // TODO switch from AVERROR_EOF to AVERROR_EOS
> +            // -32 AVERROR_EOF for avi, -51 for ogg
> +
> +                av_log(ic, AV_LOG_DEBUG, "Switching stream %d to %d\n", stream_index, ctx->pe_curidx+1);
> +                ctx->durations[ctx->pe_curidx] = ic->duration;
> +                ctx->pe_curidx = av_playlist_stream_index_from_time(ctx,
> +                                                                    av_playlist_time_offset(ctx->durations, ctx->pe_curidx),
> +                                                                    NULL);
> +                av_playlist_populate_context(ctx, ctx->pe_curidx);
> +                av_playlist_set_streams(s);
> +                // have_switched_streams is set to avoid infinite loop
> +                have_switched_streams = 1;
> +                // duration is updated in case it's checked by a parent demuxer (chained concat demuxers)
> +                s->duration = 0;
> +                for (i = 0; i < ctx->pe_curidx; ++i)
> +                    s->duration += ctx->durations[i];
> +                continue;
> +            } else {
> +                av_log(ic, AV_LOG_ERROR, "Packet read error %d\n", ret);
> +                break;

does this handle EAGAIN correctly?


[...]
> +int64_t ff_concatgen_read_timestamp(AVFormatContext *s,
> +                                    int stream_index,
> +                                    int64_t *pos,
> +                                    int64_t pos_limit)
> +{
> +    AVPlaylistContext *ctx;
> +    AVFormatContext *ic;
> +    ctx = s->priv_data;
> +    ic = ctx->icl[ctx->pe_curidx];
> +    if (ic->iformat->read_timestamp)
> +        return ic->iformat->read_timestamp(ic, stream_index, pos, pos_limit);
> +    return 0;
> +}

this could lead to problems because the correct code in utils expect a
non NULL read_timestamp() to work while here it might work just with
some child demuxers


> +
> +int ff_concatgen_read_close(AVFormatContext *s)
> +{
> +    AVPlaylistContext *ctx;
> +    AVFormatContext *ic;
> +    ctx = s->priv_data;
> +    ic = ctx->icl[ctx->pe_curidx];
> +    if (ic->iformat->read_close)
> +        return ic->iformat->read_close(ic);
> +    return 0;
> +}

While i agree that it makes sense to keep just 1 child demuxer open at a
time, this is not possible through the current demuxer API.
-> change the API
OR
-> keep all open through the demuxer interface but allow single child
   for the directly used playlist API
OR
-> possibly there are other solutions ....


[...]
> --- /dev/null
> +++ b/libavformat/concat.h
> @@ -0,0 +1,41 @@
> +/*
> + * Minimal playlist/concatenation demuxer
> + * Copyright (c) 2009 Geza Kovacs
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/** @file libavformat/concat.h
> + *  @author Geza Kovacs ( gkovacs mit edu )
> + *
> + *  @brief Minimal playlist/concatenation demuxer
> + *
> + *  @details This is a minimal concat-type demuxer that can be constructed
> + *  by allocating a PlayElem for each playlist item and setting its filename,
> + *  then allocating a PlaylistContext, creating a list of PlayElem, and setting
> + *  the PlaylistContext in the AVFormatContext.
> + */
> +
> +#ifndef AVFORMAT_CONCAT_H
> +#define AVFORMAT_CONCAT_H
> +
> +#include "concatgen.h"
> +
> +AVInputFormat* ff_concat_alloc_demuxer(void);
> +
> +#endif /* AVFORMAT_CONCAT_H */

thats a rather short header, is it really usefull?


[...]
> @@ -1248,8 +1250,15 @@ static int output_packet(AVInputStream *ist, int ist_index,
>      static unsigned int samples_size= 0;
>      AVSubtitle subtitle, *subtitle_to_free;
>      int got_subtitle;
> +    int stream_offset = 0;
>      AVPacket avpkt;
> +    AVPlaylistContext *pl_ctx = av_playlist_get_context(ic);
>  
> +    if (pl_ctx && pkt) {
> +        ist->st = ic->streams[pkt->stream_index];
> +        stream_offset = pkt->stream_index - av_playlist_localstidx_from_streamidx(pl_ctx, pkt->stream_index);
> +    }

i belive this is not "possible", some codecs like some audio codecsmay have
rather small packets and searching through a 1000 entry playlist for each
packet could lead to a noticeable slowdown


> +     

tabs are still forbidden in svn


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090823/786825ec/attachment.pgp>



More information about the ffmpeg-devel mailing list