[FFmpeg-devel] [PATCH] Playlist API
Michael Niedermayer
michaelni
Sun Aug 16 22:13:36 CEST 2009
On Thu, Aug 13, 2009 at 02:27:43PM -0700, Geza Kovacs wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 08/07/2009 06:00 PM, Michael Niedermayer wrote:
> >> I had something along these lines implemented before I went for the
> >> current stream-switching model, basically it was like this:
> >>
> >> Rather than replacing the existing streams when opening a new playlist
> >> element, nb_streams is incremented by the number of new streams, and the
> >> new streams are appended on the end of the streams list in the same
> >> order they were organized in. For example:
> >>
> >> file1 with 2 streams is opened:
> >> nb_streams=2
> >> streams[0]=file1-stream0
> >> streams[1]=file1-stream1
> >>
> >> file2 with 3 streams is opened:
> >> nb_streams=5
> >> streams[0]=file1-stream0
> >> streams[1]=file1-stream1
> >> streams[2]=file2-stream0
> >> streams[3]=file2-stream1
> >> streams[4]=file2-stream2
> >>
> >> file3 with 1 stream is opened:
> >> nb_streams=6
> >> streams[0]=file1-stream0
> >> streams[1]=file1-stream1
> >> streams[2]=file2-stream0
> >> streams[3]=file2-stream1
> >> streams[4]=file2-stream2
> >> streams[5]=file2-stream0
> > ^
> > typo :)
> >
> >
> >>
> >> ...etc...
> >>
> >> The stream index thus allows each stream to be uniquely identified; just
> >> add a nb_streams_list to the PlaylistContext and store the nb_streams of
> >> each element (so that it will work even after the AVFormatContext is
> >> freed); and by going through the list you can calculate the offset and
> >> actual stream index.
> >>
> >
> >> Now the problem with this is of course that you'll hit MAX_STREAMS
> >> rather quickly,
> >
> > well, yes, MAX_STREAMS should really be droped and replaced by a dynamically
> > allocated array, if your code depends on that you will have to implement that
> > unless someone has already done it and i forgot it ...
> > this does not seem like that much work, and it is something that will make
> > some peoplr happy even without playlists, we have at least one bug on roundup
> > due to it IIRC
> >
>
> This updated patch switches to the model described above, so that stream
> indexes uniquely identify streams rather than having streams be replaced
> on the go (though the older behavior can be restored by having
> ff_playlist_streams_offset_from_playidx always return 0).
>
> Additionally this patch fixes some packet-loss issues on certain formats
> while switching streams which existed in the previous patch.
>
> Some caveats:
>
> Number of possible playlist items is still limited by
> AVFormatContext->filename being a fixed-size buffer. I will address this
> issue, and will finish up aurel's MAX_STREAMS fix, in separate patches.
good
[...]
> +PlaylistContext *ff_playlist_get_context(AVFormatContext *ic)
> +{
> + if (ic && ic->iformat && ic->iformat->long_name && ic->priv_data &&
> + !strncmp(ic->iformat->long_name, "CONCAT", 6))
> + return ic->priv_data;
> + else
> + return NULL;
> +}
> +
> +void ff_playlist_set_context(AVFormatContext *ic, PlaylistContext *ctx)
> +{
> + if (ic && ctx)
> + ic->priv_data = ctx;
> +}
this function makes no sense
> +
> +void ff_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);
> + }
> + }
> + sepidx[len] = ts-s;
> + ts = s;
> + *len_ptr = len;
> + *flist_ptr = flist = av_malloc(sizeof(*flist) * (len+1));
> + flist[len] = 0;
> + for (i = 0; i < len; ++i) {
> + flist[i] = av_malloc(sepidx[i+1]-sepidx[i]);
> + av_strlcpy(flist[i], ts+sepidx[i], sepidx[i+1]-sepidx[i]);
> + }
> + av_free(sepidx);
> +}
missing alloc failure checks
> +
> +PlaylistContext *ff_playlist_from_encodedstring(const char *s, const char sep)
> +{
> + PlaylistContext *ctx;
> + char **flist;
> + int i, len;
> + ff_playlist_split_encodedstring(s, sep, &flist, &len);
> + if (len <= 1) {
> + for (i = 0; i < len; ++i)
> + av_free(flist[i]);
> + av_free(flist);
> + return NULL;
> + }
> + ctx = av_mallocz(sizeof(*ctx));
> + for (i = 0; i < len; ++i)
> + ff_playlist_add_path(ctx, flist[i]);
> + return ctx;
missing ctx == NULL check
> +}
> +
> +void ff_playlist_add_path(PlaylistContext *ctx, const char *itempath)
> +{
> + ctx->flist = av_realloc(ctx->flist, sizeof(*(ctx->flist)) * (++ctx->pelist_size+1));
> + ctx->flist[ctx->pelist_size] = NULL;
> + ctx->flist[ctx->pelist_size-1] = itempath;
> + ctx->durations = av_realloc(ctx->durations,
> + sizeof(*(ctx->durations)) * (ctx->pelist_size+1));
> + ctx->durations[ctx->pelist_size] = 0;
> + ctx->nb_streams_list = av_realloc(ctx->nb_streams_list,
> + sizeof(*(ctx->nb_streams_list)) * (ctx->pelist_size+1));
> + ctx->nb_streams_list[ctx->pelist_size] = 0;
> +}
> +
> +void ff_playlist_relative_paths(char **flist,
> + const 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]);
> + fullfpath = av_malloc(sizeof(char) * (wdslen+flslen+2));
sizeof(char) == 1
also declaration and init can be merged
> + 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 ff_playlist_time_offset(int64_t *durations, const int pe_curidx)
durations should be const, for the int it makes little sense, actually the
code could be simplified where it not const
> +{
> + 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?
[...]
> +/** @struct PlaylistContext
> + * @brief Represents the playlist and contains PlayElem for each playlist item.
> + */
> +typedef struct PlaylistContext {
> + char **flist; /**< List of file names for each playlist item */
> + AVFormatContext **icl; /**< List of FormatContext for each playlist items */
> + 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 */
the comments can be vertically aligned
> + int64_t *durations; /**< Durations, in AV_TIME_BASE units, for each playlist item */
why is AVFormatContext.duration not used here?
> + int *nb_streams_list; /**< List of the number of streams in each playlist item*/
why is AVFormatContext.nb_streams not used here?
> +} 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);
[...]
> +// PlaylistContext should be constructed externally
why?
> +static int concat_read_header(AVFormatContext *s,
> + AVFormatParameters *ap)
> +{
> + return 0;
> +}
> +
> +AVInputFormat* ff_concat_alloc_demuxer(void)
> +{
> + AVInputFormat *cdm = av_malloc(sizeof(*cdm));
> + cdm->name = "concat";
> + cdm->long_name = NULL_IF_CONFIG_SMALL("CONCAT format");
> + cdm->priv_data_size = sizeof(PlaylistContext);
> + cdm->read_probe = concat_probe;
> + cdm->read_header = concat_read_header;
> + cdm->read_packet = ff_concatgen_read_packet;
> + cdm->read_close = ff_concatgen_read_close;
> + cdm->read_seek = ff_concatgen_read_seek;
> + cdm->read_timestamp = ff_concatgen_read_timestamp;
> + cdm->flags = 0;
> + cdm->extensions = NULL;
> + cdm->value = 0;
> + cdm->read_play = ff_concatgen_read_play;
> + cdm->read_pause = ff_concatgen_read_pause;
> + cdm->codec_tag = codec_concat_tags;
> + cdm->read_seek2 = NULL;
> + cdm->metadata_conv = NULL;
> + cdm->next = NULL;
> + return cdm;
> +}
> +
> +AVInputFormat concat_demuxer = {
> + "concat",
> + NULL_IF_CONFIG_SMALL("CONCAT format"),
> + 0,
> + concat_probe,
> + concat_read_header,
> + ff_concatgen_read_packet,
> + ff_concatgen_read_close,
> + ff_concatgen_read_seek,
> + ff_concatgen_read_timestamp,
> + 0, //flags
> + NULL, //extensions
> + 0, //value
> + ff_concatgen_read_play,
> + ff_concatgen_read_pause,
> + (const AVCodecTag* const []){codec_concat_tags, 0},
> + NULL, //concat_read_seek2
> + NULL, //metadata_conv
> + NULL, //next
> +};
trailing NULLs are redundant
[...]
> +static int m3u_probe(AVProbeData *p)
> +{
> + if (p->buf != 0) {
> + if (!strncmp(p->buf, "#EXTM3U", 7))
> + return AVPROBE_SCORE_MAX;
> + else
> + return 0;
> + }
> + if (match_ext(p->filename, "m3u"))
> + return AVPROBE_SCORE_MAX/2;
> + else
> + return 0;
> +}
why is there a probe case for buf == NULL ?
[...]
> +static int m3u_read_header(AVFormatContext *s,
> + AVFormatParameters *ap)
> +{
> + PlaylistContext *ctx = av_mallocz(sizeof(*ctx));
> + m3u_list_files(s->pb, ctx, s->filename);
> + s->priv_data = ctx;
looks like a memleak
[...]
> +static int pls_list_files(ByteIOContext *b, PlaylistContext *ctx, const char *filename)
> +{
> + int i, j, k, c;
> + unsigned int buflen;
> + char state;
> + char **flist;
> + char buf[1024];
> + char s[5] = {0};
> + char t[] = "\nFile";
> + flist = NULL;
> + state = buflen = i = j = 0;
> + while ((c = url_fgetc(b))) {
> + if (c == EOF)
> + break;
> + if (state == 0) {
> + s[0] = s[1];
> + s[1] = s[2];
> + s[2] = s[3];
> + s[3] = s[4];
> + s[4] = c;
> + if (s[0] == t[0] && s[1] == t[1] && s[2] == t[2] && s[3] == t[3] && s[4] == t[4])
> + state = 1;
> + } else if (state == 1) {
> + if (c == '=')
> + state = 2;
> + else if (c == '#')
> + state = 0;
> + } else {
> + if (c == '\n' || c == '#') {
> + termfn:
> + buf[i++] = 0;
> + flist = av_fast_realloc(flist, &buflen, sizeof(*flist) * (j+2));
> + flist[j] = av_malloc(i);
> + av_strlcpy(flist[j++], buf, i);
> + i = 0;
> + state = 0;
> + s[sizeof(s)-1] = c;
> + continue;
> + } else {
> + buf[i++] = c;
> + if (i >= sizeof(buf)-1)
> + goto termfn;
> + }
> + }
> + }
> + if (!flist) // no files have been found
> + return AVERROR_EOF;
> + flist[j] = 0;
> + ff_playlist_relative_paths(flist, j, dirname(filename));
> + for (k = 0; k < j; ++k)
> + ff_playlist_add_path(ctx, flist[k]);
> + av_free(flist);
> + return 0;
> +}
[...]
> +static int xspf_list_files(ByteIOContext *b, PlaylistContext *ctx, const char *filename)
> +{
> + int i, j, k, c;
> + unsigned int buflen;
> + char state;
> + char **flist;
> + char buf[1024];
> + char s[10] = {0};
> + char t[] = "<location>";
> + flist = NULL;
> + state = buflen = i = j = 0;
> + while ((c = url_fgetc(b))) {
> + if (c == EOF)
> + break;
> + if (state == 0) {
> + s[0] = s[1];
> + s[1] = s[2];
> + s[2] = s[3];
> + s[3] = s[4];
> + s[4] = s[5];
> + s[5] = s[6];
> + s[6] = s[7];
> + s[7] = s[8];
> + s[8] = s[9];
> + s[9] = c;
> + if (s[0] == t[0] && s[1] == t[1] && s[2] == t[2] && s[3] == t[3] && s[4] == t[4] &&
> + s[5] == t[5] && s[6] == t[6] && s[7] == t[7] && s[8] == t[8] && s[9] == t[9])
> + state = 1;
> + } else {
> + if (c == '<') {
> + termfn:
> + buf[i++] = 0;
> + flist = av_fast_realloc(flist, &buflen, sizeof(*flist) * (j+2));
> + flist[j] = av_malloc(i);
> + av_strlcpy(flist[j++], buf, i);
> + i = 0;
> + state = 0;
> + s[sizeof(s)-1] = c;
> + continue;
> + } else {
> + buf[i++] = c;
> + if (i >= sizeof(buf)-1)
> + goto termfn;
> + }
> + }
> + }
> + if (!flist) // no files have been found
> + return AVERROR_EOF;
> + flist[j] = 0;
> + ff_playlist_relative_paths(flist, j, dirname(filename));
> + for (k = 0; k < j; ++k)
> + ff_playlist_add_path(ctx, flist[k]);
> + av_free(flist);
> + return 0;
> +}
that code looks duplciated
[...]
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 43147a5..c9ec405 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -672,7 +672,7 @@ int avcodec_close(AVCodecContext *avctx)
>
> if (HAVE_THREADS && avctx->thread_opaque)
> avcodec_thread_free(avctx);
> - if (avctx->codec->close)
> + if (avctx->codec && avctx->codec->close)
> avctx->codec->close(avctx);
> avcodec_default_free_buffers(avctx);
> av_freep(&avctx->priv_data);
when is avctx->codec == NULL ?
> diff --git a/ffmpeg.c b/ffmpeg.c
> index e5512c2..4b1fbdc 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -40,6 +40,7 @@
> #include "libavutil/fifo.h"
> #include "libavutil/avstring.h"
> #include "libavformat/os_support.h"
> +#include "libavformat/concat.h"
either concat.h is a public header or not
if it is, the things in it are public API, need AV prefixes, be well designed,
the header would need to be installed, would only be allowed to itself include
public headers, ...
OR
ffmpeg.c can not #include it
>
> #if HAVE_SYS_RESOURCE_H
> #include <sys/types.h>
> @@ -1232,7 +1233,8 @@ static void print_report(AVFormatContext **output_files,
> /* pkt = NULL means EOF (needed to flush decoder buffers) */
> static int output_packet(AVInputStream *ist, int ist_index,
> AVOutputStream **ost_table, int nb_ostreams,
> - const AVPacket *pkt)
> + AVPacket *pkt,
> + AVFormatContext *ic)
> {
> AVFormatContext *os;
> AVOutputStream *ost;
> @@ -1244,8 +1246,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;
> + PlaylistContext *pl_ctx = ff_playlist_get_context(ic);
>
> + if (pl_ctx && pkt) {
> + ist->st = ic->streams[pkt->stream_index];
> + stream_offset = pkt->stream_index - ff_playlist_localstidx_from_streamidx(pl_ctx, pkt->stream_index);
> + }
> +
> if(ist->next_pts == AV_NOPTS_VALUE)
> ist->next_pts= ist->pts;
>
tabs are forbidden in svn
> @@ -1287,6 +1296,7 @@ static int output_packet(AVInputStream *ist, int ist_index,
> endianness as CPU */
> ret = avcodec_decode_audio3(ist->st->codec, samples, &data_size,
> &avpkt);
> +
> if (ret < 0)
> goto fail_decode;
> avpkt.data += ret;
cosmetics
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20090816/1f451db4/attachment.pgp>
More information about the ffmpeg-devel
mailing list