[FFmpeg-devel] [PATCH] Playlist API

Geza Kovacs gkovacs
Mon Aug 24 10:16:09 CEST 2009


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
> 

Storing a single sum of durations would prevent the API from allowing
the user to seek backwards in the playlist. For example, given the
following set of playlist element durations:

1. 300
2. 400
3. 200
4. 600
5. 500
6. 100

Then if I'm at 1600 (element 5) and wish to go back 800, then with just
a single sum I'll know it's somewhere between elements 1-4 but would
need to re-check (open and check duration of) all the previous ones to
pinpoint the actual one.

Alternatively, if you meant store an array of sums, that would work for
seeking backwards, 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.
At any rate I've introduced a caching mechanism into those functions
which should minimize the amount of calculation needed for each packet.

> 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.

> 
> [...]
>> +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?

>> +
>> +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 - 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).

> 
> [...]
>> @@ -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
> 

As mentioned above, I'm now using a caching mechanism for the
av_playlist functions which are called every time a packet is read and
output, hence the packet-read performance shouldn't suffer even with
larger playlists.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-concat.diff
Type: text/x-diff
Size: 57152 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090824/d84803eb/attachment.diff>



More information about the ffmpeg-devel mailing list