[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