[FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

Nicolas George george at nsup.org
Tue Aug 2 18:14:20 EEST 2016


Le primidi 11 thermidor, an CCXXIV, sebechlebskyjan at gmail.com a écrit :
> From: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
> 
> ---
>  libavcodec/avcodec.h |  74 ++++++++++++++
>  libavcodec/bsf.c     | 284 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 358 insertions(+)

Looks rather nice. Comments below.

> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 36f7935..39106ee 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -5949,6 +5949,80 @@ void av_bsf_free(AVBSFContext **ctx);
>   */
>  const AVClass *av_bsf_get_class(void);
>  
> +/**
> + * Structure for chain/list of bitstream filters.
> + * Empty list can be allocated by av_bsf_list_alloc().
> + */
> +typedef struct AVBSFList AVBSFList;
> +
> +/**
> + * Allocate empty list of bitstream filters.
> + * The list must be later freed by av_bsf_list_free()
> + * or finalized by av_bsf_list_finalize().
> + *
> + * @return pointer to AVBSFList on success, NULL in case of failure
> + */

> +AVBSFList *av_bsf_list_alloc(void);

This is personal, but for new APIs, I like "int foo_alloc(Foo **rfoo)"
better than "Foo *foo_alloc(void)": that way, the caller can forward the
error code instead of guessing it is ENOMEM.

> +
> +/**
> + * Free list of bitstream filters.
> + *
> + * @param lst pointer to pointer returned by av_bsf_list_alloc()
> + */
> +void av_bsf_list_free(AVBSFList **lst);
> +

> +/**
> + * Append bitstream filter to the list of bitstream filters.
> + *
> + * @param lst list to append to
> + * @param bsf AVBSFContext to be appended
> + *
> + * @return >=0 on success, negative AVERROR in case of failure
> + */
> +int av_bsf_list_append(AVBSFList *lst, AVBSFContext *bsf);

An utility function that combines av_bsf_get_by_name(), av_bsf_alloc() and
this av_bsf_list_append() may be nice to have too.

> +
> +/**
> + * Finalize list of bitstream filters.
> + *
> + * This function will transform AVBSFList to single AVBSFContext,
> + * so the whole chain of bitstream filters can be treated as single filter
> + * freshly allocated by av_bsf_alloc().
> + * The AVBSFList structure is destroyed after this call and must not
> + * be accessed.
> + *

> + * @param lst      AVBSFList structure to be transformed
> + * @param bsf[out] pointer to be set to newly created AVBSFContext structure
> + *                 representing the chain of bitstream filters

Maybe document that lst no longer belongs to the caller if the call succeeds
and what happens to it if it fails.

> + *
> + * @return >=0 on success, negative AVERROR in case of failure
> + */
> +int av_bsf_list_finalize(AVBSFList **lst, AVBSFContext **bsf);
> +
> +/**
> + * Parse string describing list of bitstream filters and create single
> + * AVBSFContext describing the whole chain of bitstream filters.
> + * Resulting AVBSFContext can be treated as any other AVBSFContext freshly
> + * allocated by av_bsf_alloc().
> + *
> + * @param str      string describing chain of bitstream filters in format
> + *                 bsf1[=opt1=val1:opt2=val2][,bsf2]
> + * @param bsf[out] pointer to be set to newly created AVBSFContext structure
> + *                 representing the chain of bitstream filters
> + *
> + * @return >=0 on success, negative AVERROR in case of failure
> + */
> +int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf);
> +
> +/**
> + * Get null/pass-through bitstream filter.
> + *
> + * @param bsf[out] pointer to be set to new instance of pass-through
> + *                 bitstream filter
> + *
> + * @return
> + */
> +int av_bsf_get_null_filter(AVBSFContext **bsf);
> +
>  /* memory */
>  
>  /**
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index 40fc925..3ae0a2b 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -22,6 +22,7 @@
>  #include "libavutil/mem.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/avassert.h"
> +#include "libavutil/avstring.h"
>  
>  #include "avcodec.h"
>  #include "bsf.h"
> @@ -236,3 +237,286 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
>  
>      return 0;
>  }
> +
> +typedef struct BSFListContext {
> +    const AVClass *class;
> +

> +    AVBSFContext **bsf_lst;
> +    int ctx_nr;

The existing code uses "foo" and "nb_foo", or sometimes "foo" and
"foo_count" in this kind of case.

> +
> +    int idx;           // index of currently processed BSF
> +    int flushed_idx;   // index of BSF being flushed

Also, probably better make these unsigned.

> +
> +} BSFListContext;
> +
> +
> +static int bsf_list_init(AVBSFContext *bsf)
> +{
> +    BSFListContext *lst = bsf->priv_data;
> +    int ret, i;
> +    const AVCodecParameters *cod_par = bsf->par_in;
> +    AVRational tb = bsf->time_base_in;
> +
> +    for (i = 0; i < lst->ctx_nr; ++i) {
> +        ret = avcodec_parameters_copy(lst->bsf_lst[i]->par_in, cod_par);
> +        if (ret < 0)
> +            goto fail;
> +
> +        lst->bsf_lst[i]->time_base_in = tb;
> +
> +        ret = av_bsf_init(lst->bsf_lst[i]);
> +        if (ret < 0)
> +            goto fail;
> +
> +        cod_par = lst->bsf_lst[i]->par_out;
> +        tb = lst->bsf_lst[i]->time_base_out;
> +    }
> +
> +    bsf->time_base_out = tb;
> +    ret = avcodec_parameters_copy(bsf->par_out, cod_par);
> +
> +fail:
> +    return ret;
> +}
> +
> +static int bsf_list_flush(AVBSFContext *bsf, AVPacket *out)
> +{
> +    BSFListContext *lst = bsf->priv_data;
> +    int ret;
> +
> +    if (lst->flushed_idx == lst->ctx_nr)
> +        return AVERROR_EOF;
> +
> +    while (1) {
> +        if (lst->idx > lst->flushed_idx) {
> +            ret = av_bsf_receive_packet(lst->bsf_lst[lst->idx-1], out);
> +            if (ret == AVERROR(EAGAIN)) {
> +                ret = 0;
> +                lst->idx--;
> +                continue;
> +            } else if (ret == AVERROR_EOF) {
> +                /* filter idx-1 is done, let's flush filter idx */
> +                lst->flushed_idx = lst->idx;
> +                continue;
> +            } else if (ret < 0) {
> +                /* filtering error */
> +                break;
> +            }
> +        }
> +
> +        if (lst->idx < lst->ctx_nr) {
> +            AVPacket *pkt = (lst->idx == lst->flushed_idx) ? NULL : out;
> +            ret = av_bsf_send_packet(lst->bsf_lst[lst->idx], pkt);
> +            if (ret < 0)
> +                break;
> +            lst->idx++;
> +        } else {
> +            /* The end of filter chain, break to return result */
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +static int bsf_list_filter(AVBSFContext *bsf, AVPacket *out)
> +{
> +    BSFListContext *lst = bsf->priv_data;
> +    int ret;
> +
> +    if (!lst->ctx_nr)
> +        return ff_bsf_get_packet_ref(bsf, out);
> +
> +    while (1) {
> +        if (lst->idx) {
> +            ret = av_bsf_receive_packet(lst->bsf_lst[lst->idx-1], out);
> +            if (ret == AVERROR(EAGAIN)) {
> +                ret = 0;
> +                lst->idx--;
> +                continue;
> +            } else if (ret < 0) {
> +                break;
> +            }
> +        } else {
> +            ret = ff_bsf_get_packet_ref(bsf, out);
> +            if (ret < 0)
> +                break;
> +        }
> +
> +        if (lst->idx < lst->ctx_nr) {
> +            ret = av_bsf_send_packet(lst->bsf_lst[lst->idx], out);
> +            if (ret < 0)
> +                break;
> +            lst->idx++;
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    if (ret == AVERROR_EOF)
> +        ret = bsf_list_flush(bsf, out);
> +
> +    if (ret < 0)
> +        av_packet_unref(out);
> +
> +    return ret;
> +}

These two functions are very similar. It would be better to share the code
as much as possible. Otherwise, a bug fixed in one will stay in the other.

> +
> +static void bsf_list_close(AVBSFContext *bsf)
> +{
> +    BSFListContext *lst = bsf->priv_data;
> +    int i;
> +
> +    for (i = 0; i < lst->ctx_nr; ++i)
> +        av_bsf_free(&lst->bsf_lst[i]);
> +    av_freep(&lst->bsf_lst);
> +}
> +

> +static const AVClass bsf_list_class = {
> +        .class_name = "bsf_list",
> +        .item_name = av_default_item_name,
> +        .version = LIBAVUTIL_VERSION_INT,
> +};
> +
> +const AVBitStreamFilter ff_list_bsf = {
> +        .name = "bsf_list",
> +        .priv_data_size = sizeof(BSFListContext),
> +        .priv_class = &bsf_list_class,
> +        .init = bsf_list_init,
> +        .filter = bsf_list_filter,
> +        .close = bsf_list_close,
> +};

A little alignment would be nice.

> +
> +typedef struct AVBSFList {
> +    AVBSFContext **bsf_lst;
> +    int ctx_nr;
> +} AVBSFList;

I think it would be possible to use the same structure for AVBSFList and
BSFListContext. Sure, AVBSFList would have extra fields, but that is no
matter, since its only use is to be eventually upgraded into a
BSFListContext.

The benefit would be to avoid a malloc()/copy/free() cycle: just declare
.priv_data_size = 0 to prevent lavc from allocating the private structure,
and set priv_data directly.

> +
> +AVBSFList *av_bsf_list_alloc(void)
> +{
> +    return av_mallocz(sizeof(AVBSFList));
> +}
> +
> +void av_bsf_list_free(AVBSFList **lst)
> +{

if (!*lst)
    return;

> +    int i;
> +    for (i = 0; i < (*lst)->ctx_nr; ++i)
> +        av_bsf_free(&(*lst)->bsf_lst[i]);
> +    av_free((*lst)->bsf_lst);
> +    av_freep(lst);
> +}
> +
> +int av_bsf_list_append(AVBSFList *lst, AVBSFContext *bsf)
> +{
> +    return av_dynarray_add_nofree(&lst->bsf_lst, &lst->ctx_nr, bsf);
> +}
> +
> +int av_bsf_list_finalize(AVBSFList **lst, AVBSFContext **bsf)
> +{
> +    int ret = 0;
> +    BSFListContext *ctx;
> +
> +    if ((*lst)->ctx_nr == 1) {
> +        *bsf = (*lst)->bsf_lst[0];

> +        (*lst)->bsf_lst = NULL;

I think you are leaking bsf_lst here.

> +        (*lst)->ctx_nr = 0;
> +        goto end;
> +    }
> +
> +    ret = av_bsf_alloc(&ff_list_bsf, bsf);
> +    if (ret < 0)
> +        return ret;
> +
> +    ctx = (*bsf)->priv_data;
> +
> +    ctx->bsf_lst = (*lst)->bsf_lst;
> +    ctx->ctx_nr = (*lst)->ctx_nr;
> +
> +end:
> +    av_freep(lst);
> +    return ret;
> +}
> +
> +static int bsf_parse_str(const char *str, AVBSFContext **bsf)
> +{
> +    const AVBitStreamFilter *filter;
> +    char *bsf_name, *bsf_options_str, *buf;
> +    int ret = 0;
> +
> +    if (!(buf = av_strdup(str)))
> +        return AVERROR(ENOMEM);
> +
> +    bsf_name = av_strtok(buf, "=", &bsf_options_str);
> +    if (!bsf_name) {
> +        ret = AVERROR(EINVAL);
> +        goto end;
> +    }
> +
> +    filter = av_bsf_get_by_name(bsf_name);
> +    if (!filter) {
> +        ret = AVERROR_BSF_NOT_FOUND;
> +        goto end;
> +    }
> +
> +    ret = av_bsf_alloc(filter, bsf);
> +    if (ret < 0)
> +        goto end;
> +
> +    if (bsf_options_str) {
> +        ret = av_set_options_string(*bsf, bsf_options_str, "=", ":");
> +        if (ret < 0)
> +            av_bsf_free(bsf);
> +    }
> +
> +end:
> +    av_free(buf);
> +    return ret;
> +}
> +
> +int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf_lst)
> +{
> +    AVBSFList *lst;
> +    char *bsf_str, *buf, *dup, *saveptr;
> +    int ret;
> +
> +    if (!str)
> +        return av_bsf_get_null_filter(bsf_lst);
> +
> +    lst = av_bsf_list_alloc();
> +    if (!lst)
> +        return AVERROR(ENOMEM);
> +
> +    if (!(dup = buf = av_strdup(str)))
> +        return AVERROR(ENOMEM);
> +
> +    while (1) {
> +        AVBSFContext *bsf;
> +        bsf_str = av_strtok(buf, ",", &saveptr);
> +        if (!bsf_str)
> +            break;
> +
> +        ret = bsf_parse_str(bsf_str, &bsf);
> +        if (ret < 0)
> +            goto end;
> +
> +        ret = av_bsf_list_append(lst, bsf);
> +        if (ret < 0) {
> +            av_bsf_free(&bsf);
> +            goto end;
> +        }
> +
> +        buf = NULL;
> +    }
> +
> +    ret = av_bsf_list_finalize(&lst, bsf_lst);
> +end:
> +    if (ret < 0)
> +        av_bsf_list_free(&lst);
> +    av_free(dup);
> +    return ret;
> +}
> +
> +int av_bsf_get_null_filter(AVBSFContext **bsf)
> +{
> +    return av_bsf_alloc(&ff_list_bsf, bsf);
> +}

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160802/78ead07d/attachment.sig>


More information about the ffmpeg-devel mailing list