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

From: Jan Sebechlebsky <sebechlebskyjan@gmail.com>

---
 libavcodec/avcodec.h |  74 ++++++++++++++
 libavcodec/bsf.c     | 284 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 358 insertions(+)

Le primidi 11 thermidor, an CCXXIV, sebechlebskyjan@gmail.com a écrit :
> From: Jan Sebechlebsky <sebechlebskyjan@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,

On 8/2/2016 12:14 PM, Nicolas George wrote:
>> +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.

There is no other error this could generate. It's literally an
av_mallocz wrapper.
Every other similar alloc() function in avcodec.h (with no extra
parameters that could generate EINVAL errors and such) also have
this signature, so lets keep try to keep it consistent.

Sorry, I seem to have lost the original mail, and as Gmane is dead I had to
review using Nicolas's mail.

On Tue, Aug 2, 2016 at 8:14 AM Nicolas George <george@nsup.org> wrote:

> Le primidi 11 thermidor, an CCXXIV, sebechlebskyjan@gmail.com a écrit :
> > 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
>

For  Doxygen documentation, capitalize the first letter of the descriptions
in @return's and @param's, and @ref-erence types, structs, and enums. (For
functions, an empty set of parentheses is enough.)

The same rules apply to all the new functions you've added.

@return Pointer to @ref AVBSFList on success, NULL in case of failure.


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

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

List


> > + * @param bsf AVBSFContext to be appended
>

@ref AVBSFContext


> > + *
> > + * @return >=0 on success, negative AVERROR in case of failure
> > + */
> > +int av_bsf_list_append(AVBSFList *lst, AVBSFContext *bsf);
> > +
> > +/**
> > + * Finalize list of bitstream filters.
> > + *
> > + * This function will transform AVBSFList to single AVBSFContext,
>

@ref's


> > + * 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
>

Ditto


> > + * be accessed.
> > + *
> > + * @param lst      AVBSFList structure to be transformed
>


> > + * @param bsf[out] pointer to be set to newly created AVBSFContext
> structure
>

@param[out] bsf

Same applies to rest of the file.


> > + *                 representing the chain of bitstream filters
> > + *
> > + * @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]
>

Use Markdown backtick syntax to make it display in monospace font

`bsf[=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 */
> >
> >  /**
>

Hello Nicolas,

On 08/02/2016 05:14 PM, Nicolas George wrote:
>
>> +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.
I left that as it is, since James commented this is more common way.
>
>> +
>> +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.
I think it is nicer the way it is - I do not like an idea of manually 
assigning private data when priv_data_size will be set to 0, it seems 
like something which can potentially create problem in future. Also the 
overhead is negligible here, since bsf list finalization will be done 
only once or few times per whole transcoding.

I've fixed all other issues you suggested.

Thanks for review!

Regards,
Jan

On 08/02/2016 05:26 PM, James Almer wrote:
> On 8/2/2016 12:14 PM, Nicolas George wrote:
>>> +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.
> There is no other error this could generate. It's literally an
> av_mallocz wrapper.
> Every other similar alloc() function in avcodec.h (with no extra
> parameters that could generate EINVAL errors and such) also have
> this signature, so lets keep try to keep it consistent.
> _______________________________________________
I'll keep it this way then :)

Regards,
Jan