[FFmpeg-devel] [PATCH] Playlist API
Michael Niedermayer
michaelni
Mon Aug 24 12:54:01 CEST 2009
On Mon, Aug 24, 2009 at 01:16:09AM -0700, Geza Kovacs wrote:
> Attached patch fixes most of the issues you pointed out.
>
> On 08/22/2009 08:15 PM, Michael Niedermayer wrote:
> >> +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
> >
[...]
> Alternatively, if you meant store an array of sums, that would work for
> seeking backwards,
that was what i meant
> but if the playlist is large enough that it actually
> makes a significant performance difference, then there could potentially
> be a risk of overflow over the int64_t if the playlist is really long.
a playlist 600k years long?
I guess its not impossible but other things will fail at that point as well
seeking being one, the whole file duration the next
> At any rate I've introduced a caching mechanism into those functions
> which should minimize the amount of calculation needed for each packet.
you cant add a static cache, it breaks multithreaded apps
besides, even with a cache i think bsearch() should be used when the
cache fails.
and you should not linearly search through the cache ...
that said, isnt just keeping track of which file was read from last
enough to do O(1) accesses for sequential playback? (that obviously
requires the summed duration array)
>
> > 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?
> >
>
> Other than the stream index manipulation functions which are needed for
> backwards compatibility with the fixed-streams AVFormatContext, most of
> my API is primarily meant to do that, except with some convenience
> functions for constructing playlist demuxers from lists of files, which
> I believe is a useful abstraction for those who don't want to deal with
> constructing it manually.
my question is still the same, what is the point of a dozen functions over
a single one adding the entries of a list into a playlist.
such function could add a single file, it could add "file1,file2,..."
it could add them to an existing context and could start with NULL as
context and return a newly created one.
This does seem simpler to me, maybe it is not but you arent really
saying why you think so if you do.
>
> >
> > [...]
> >> +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
> >
>
> Ok, fixed, I'm offsetting both except when pts is AV_NOPTS_VALUE
>
> > also av_playlist_time_offset() is too slow to be run on each packet
> >
>
> I'm now using a caching mechanism for that function, as well as the
> others which are called every time packets are output, so that the full
> loop through the entire durations array will only need to be done once
> after a new item is opened.
>
> >
> >> + }
> >> + 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?
> >
>
> By "correctly" do you mean don't switch streams or did you mean
> something else?
i Just meant that demuxers can return EAGAIN and that is not an error its a
"retry later, no data yet"
>
> >> +
> >> +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 ....
> >
>
> I'll opt for the second option
good
> - ff_concatgen_read_close now closes all
> child demuxers by default, and if the application needs finer control
> over memory it can just use formatcontext_list to close and free the
> child demuxers individually and set the entries to NULL to indicate
> they've been closed.
>
> >> +#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?
> >
>
> Given that most of concat.c is static and hence the function can't be
> moved to avplaylist or concatgen, nor is it really related to either of
> the other headers, I don't think there's a much better way (except
> perhaps using extern when calling it, but I consider that rather hackish).
fine, lets forget this one its really not important
ill review the patch soon
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20090824/8e2c26d1/attachment.pgp>
More information about the ffmpeg-devel
mailing list