[FFmpeg-devel] [PATCH] Playlist API

Michael Niedermayer michaelni
Tue Aug 25 23:52:23 CEST 2009


On Tue, Aug 25, 2009 at 02:09:31AM -0700, Geza Kovacs wrote:
> On 08/24/2009 03:54 AM, Michael Niedermayer wrote:
[...]
> > 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.
> > 
> 
> Ok, av_playlist_add_filelist and av_playlist_add_path should now do
> that. Since the return value is used for the error code, the newly
> allocated AVFormatContext is accessible via
> ctx->formatcontext_list[ctx->pelist_size-1].
> 
> I'm opting for having av_playlist_alloc_concat_formatcontext allocate
> the master AVFormatContext and AVPlaylistContext beforehand since I
> don't want to have to check for null AVPlaylistContext every time it's
> used (and since it allows for ff_concat_alloc_demuxer to be kept outside
> the public headers, since it's only used when constructing a master
> AVFormatContext).

I think the API needs more work, designing an API is not just about throwing
random functions in a header. We cant change or remove these once it is in
svn because libav* must stay compatible between versions whenever possible.

currently there are 12 public functions, for some of which i do not even know
when or why a user application would call them. (-> that at the very least
means the documentation is bad ...)

It seems we agree that playlists should be accessable as a demuxer and also
through a new API as the existing demuxer API is not entirely appropriate.
That you have implemented (short of a few bugs and nitpicks...) but
the interface API seems very poor IMHO.

if we take a step back, not thinking of the implementation
* There is a playlist, that is a ordered list of multimedia files
* The obvious use cases a user application could have in mind are:
  A Create such a list
  B Destroy such a list
  C Insert a file into the list at a specific position
  D Remove a file from the list (from a specific position)
  E Get the AVFormatContext for a specific index

Now if we just have a array of AVFormatContexts, that makes E just a
playlist.avf[index]

that leaves us with 4 functions but your public header adds 12
I hope you see what my problem is with this, the public API is used by
application developers and must be simple, effective and easy to understand
and remember (our existing APIs are poor enough in some spots (improvments
for them of course are welcome but thats off topic in this thread...))

and if we consider that the insert function can be designed a little like
realloc() then it could just as well create the playlist, though maybe thats
not a good idea i dont know, it would reduce us down to 3 functions ...


> 
> ===Comments from other email===
> 
> >> +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);
> >> +    if (err < 0) {
> >> +        av_log(ic, AV_LOG_ERROR, "Error during av_open_input_file\n");
> >> +        av_free(ic);
> >> +        return NULL;
> >> +    }
> >> +    err = av_find_stream_info(ic);
> >> +    if (err < 0) {
> >> +        av_log(ic, AV_LOG_ERROR, "Could not find stream info\n");
> >> +        av_free(ic);
> >> +        return NULL;
> >> +    }
> >
> > i think this contains memleaks if av_find_stream_info fails, i guess
> > you should try valgrind on your code, maybe there are more such issues
> >
> 
> Is there anything along the lines of avformat_free_context which frees
> the AVFormatContext and all its children, kind of like a destructor in
> C++, or is there some design decision on why something like that can't
> be part of the public API? 

there is av_close_input_file()

and if you have suggestions how to simplify the API or improve its
documentation these are welcome


> Or couldn't av_find_stream_info use a 'if
> (err) goto fail' where fail cleans up after itself?

i suspect av_find_stream_info cleans up after itself, but the demuxer is
not part of av_find_stream_info, a failed av_find_stream_info() does not mean
that there is something wrong with the demuxer just that some information
could not be found, an application could choose to use the demuxer anyway


> I really don't think
> that adding a bunch of frees to clean up the AVFormatContext's members
> every time some operation fails is really elegant nor maintainable (each
> block of frees would to be changed every time AVFormatContext and its
> members have some members added or removed).
> 
> Anyhow valgrind's memcheck seemed to be warning mostly about not
> checking for realloc failures, which I've fixed.
> 
> > but i see now that there is a more tricky issue above that, namely
> > the av_playlist_time_offset is rescaled to the stream timebase and that
> > is not accurate ... for example a 1 frame per second video timebase=1
> > and a audio streams that has a 1/44100 timebase could end up with up to
> > 0.5 sec desync
> > That is, its not just a academic rounding problem.
> > But i dont know how to solve this ATM, so i suggest unless you have a
> > great idea to leave this as a known bug for later, it must be fixed but
> > i need to think about it a little first how to fix it ...
> >
> 
> Perhaps I could store the durations in native stream time base in one
> array, then have another array that stores the scaling ratios for each
> native time base, and just convert directly to the target stream time
> base when needed, thus avoiding having to convert to AV_TIME_BASE at all?

the problem is that the offsetet timestamps in general are not acurately
representable in the target timebase, AV_TIME_BASE is not the only problem
I mean if one had a 1 second long file and thus wanted to add 1 second to
the next file. Lets say that file has a 1/44100 sound track and a "29.97"fps
video you surely can add 44100 to the audio timestamps but you cannot add
29.97 (or likely exactly 30000/1001) to the video timestamps.
Iam still not sure how to handle this but i would guess at least the new
playlist API that doesnt go through a demuxer should probably leave the
timestamps as they are.


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

this search should start from pe_curidx, which would make it alot
faster in practice


> +    if (localpts)
> +        *localpts = pts-(total-ctx->durations[i-1]);
> +    return i;
> +}
> +

> +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;
> +}

like with durations these could be summed up


[...]
> +/** @brief Opens the playlist element with the specified index from the AVPlaylistContext.
> + *  @param ctx AVPlaylistContext containing the desired playlist element.
> + *  @param pe_curidx Index of the playlist element to be opened.
> + *  @return Returns 0 upon success, or negative upon failure.
> + */
> +int av_playlist_populate_context(AVPlaylistContext *ctx, int pe_curidx);

never used outside libav* so there is no point in adding it to the public API


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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20090825/ac03a269/attachment.pgp>



More information about the ffmpeg-devel mailing list