[FFmpeg-devel] [PATCH] Playlist API

[Michael Niedermayer]

On Wed, Aug 26, 2009 at 02:00:18AM -0700, Geza Kovacs wrote:
> On 08/25/2009 02:52 PM, Michael Niedermayer wrote:
> > 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
> 
> av_mallocz(sizeof(AVPlaylistContext)) -- since realloc is used to expand
> the various lists (and hence the first realloc acts as a malloc),
> there's no need to explicitly allocate anything else beforehand.

This has problems, setting defaults different from 0 is one


> 
> >   B Destroy such a list
> 
> av_playlist_close
> 
> >   C Insert a file into the list at a specific position
> 
> av_playlist_insert_item
> 
> I also added a convenience function av_playlist_add_item to add the new
> item to the end since that's probably the most common position where
> items would be inserted, I think it should be pretty harmless and
> self-explanatory but if you don't like it I can remove it.

insert() with index ctx->pelist_size could be used directly


> 
> >   D Remove a file from the list (from a specific position)
> 
> av_playlist_remove_item
> 
> >   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]
> > 
> 
> playlist->formatcontext_list[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...))
> > 
> 
> I've moved the other functions out of the public header (avplaylist.h)
> into a private header (playlist.h)

thank you, we can always make things public the reverse is not easy


[...]
> +int av_playlist_insert_item(AVPlaylistContext *ctx, const char *itempath, int pos)
> +{
> +    int i;
> +    int64_t *durations_tmp;
> +    unsigned int *nb_streams_list_tmp;
> +    char **flist_tmp;
> +    flist_tmp = av_realloc(ctx->flist, sizeof(*(ctx->flist)) * (++ctx->pelist_size+1));
> +    if (!flist_tmp) {
> +        av_log(NULL, AV_LOG_ERROR, "av_realloc error in av_playlist_insert_item\n");
> +        av_free(ctx->flist);
> +        ctx->flist = NULL;
> +        return AVERROR_NOMEM;
> +    } else
> +        ctx->flist = flist_tmp;
> +    for (i = ctx->pelist_size; i > pos; --i)
> +        ctx->flist[i] = ctx->flist[i - 1];

> +    ctx->flist[pos] = itempath;

id guess this should not directly point to the user supplied string and
require the user to keep it available?
also it should be possible for the user to add playlists through this
same api (like a .m3u) and have its contents automatically inserted


> +    ctx->flist[ctx->pelist_size] = NULL;
> +    durations_tmp = av_realloc(ctx->durations,
> +                               sizeof(*(ctx->durations)) * (ctx->pelist_size+1));
> +    if (!durations_tmp) {
> +        av_log(NULL, AV_LOG_ERROR, "av_realloc error in av_playlist_insert_item\n");
> +        av_free(ctx->durations);
> +        ctx->durations = NULL;
> +        return AVERROR_NOMEM;
> +    } else
> +        ctx->durations = durations_tmp;

a failed insert should ideally leave the list in the previous state,
that also should be simpler (no free() needed)


> +    for (i = ctx->pelist_size; i > pos; --i)
> +        ctx->durations[i] = ctx->durations[i - 1];

this loop can be merged with the previous


> +    ctx->durations[pos] = 0;
> +    ctx->durations[ctx->pelist_size] = 0;

i thought these were sums of durations?


> +    nb_streams_list_tmp = av_realloc(ctx->nb_streams_list,
> +                                     sizeof(*(ctx->nb_streams_list)) * (ctx->pelist_size+1));
> +    if (!nb_streams_list_tmp) {
> +        av_log(NULL, AV_LOG_ERROR, "av_realloc error in av_playlist_insert_item\n");
> +        av_free(ctx->nb_streams_list);
> +        ctx->nb_streams_list = NULL;
> +        return AVERROR_NOMEM;
> +    } else
> +        ctx->nb_streams_list = nb_streams_list_tmp;
> +    for (i = ctx->pelist_size; i > pos; --i)
> +        ctx->nb_streams_list[i] = ctx->nb_streams_list[i - 1];
> +    ctx->nb_streams_list[pos] = 0;
> +    ctx->nb_streams_list[ctx->pelist_size] = 0;
> +    return 0;
> +}
> +

> +int av_playlist_remove_item(AVPlaylistContext *ctx, int pos)
> +{
> +    int i;
> +    int64_t *durations_tmp;
> +    unsigned int *nb_streams_list_tmp;
> +    AVFormatContext **formatcontext_list_tmp;
> +    char **flist_tmp;
> +    if (pos >= ctx->pelist_size || !ctx->flist || !ctx->durations || !ctx->nb_streams_list)
> +        return AVERROR_INVALIDDATA;
> +    for (i = pos; i < ctx->pelist_size; ++i)
> +        ctx->flist[i] = ctx->flist[i + 1];
> +    flist_tmp = av_realloc(ctx->flist, sizeof(*(ctx->flist)) * (--ctx->pelist_size+1));
> +    if (!flist_tmp) {
> +        av_log(NULL, AV_LOG_ERROR, "av_realloc error in av_playlist_remove_item\n");
> +        av_free(ctx->flist);
> +        ctx->flist = NULL;
> +        return AVERROR_NOMEM;
> +    } else
> +        ctx->flist = flist_tmp;
> +    ctx->flist[ctx->pelist_size] = NULL;
> +    for (i = pos; i < ctx->pelist_size; ++i)
> +        ctx->durations[i] = ctx->durations[i + 1];

> +    durations_tmp = av_realloc(ctx->durations,
> +                               sizeof(*(ctx->durations)) * (ctx->pelist_size+1));

is the size not off by 1?

[...]
> +int av_playlist_close(AVPlaylistContext *ctx)
> +{
> +    int err;
> +    while (ctx->pelist_size > 0) {
> +        err = av_playlist_remove_item(ctx, ctx->pelist_size-1);
> +        if (err) {
> +            av_log(NULL, AV_LOG_ERROR, "failed to remove item %d from playlist", ctx->pelist_size-1);
> +            return err;
> +        }
> +    }

> +    if (ctx->flist)
> +        av_free(ctx->flist);
> +    if (ctx->durations)
> +        av_free(ctx->durations);
> +    if (ctx->nb_streams_list)
> +        av_free(ctx->nb_streams_list);
> +    if (ctx->formatcontext_list)
> +        av_free(ctx->formatcontext_list);

av_free(NULL) is safe, the if() is unneeded

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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- 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/20090826/3cf197f9/attachment.pgp>