[FFmpeg-devel] [PATCH 2/2] avcodec/bsf: add av_bsf_join() to chain bitstream filters

[Marton Balint]

On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:

> Marton Balint:
>> Signed-off-by: Marton Balint <cus at passwd.hu>
>> ---
>>  doc/APIchanges       |  3 +++
>>  libavcodec/avcodec.h | 19 ++++++++++++++++
>>  libavcodec/bsf.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/version.h |  2 +-
>>  4 files changed, 85 insertions(+), 1 deletion(-)
>> 
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index f1d7eac2ee..1473742139 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>
>>  API changes, most recent first:
>> 
>> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
>> +  Add av_bsf_join() to chain bitstream filters.
>> +
>>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>    av_read_frame() now guarantees to handle uninitialized input packets
>>    and to return refcounted packets on success.
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 8fc0ad92c9..2812055e8a 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt);
>>   */
>>  void av_bsf_flush(AVBSFContext *ctx);
>> 
>> +/**
>> + * Join a new bitstream filter to an already operating one.
>> + *
>> + * @param[in,out] out_bsf *out_bsf contains the existing, already initialized bitstream
>> + *                        filter, the new filter will be joined to the output of this.
>> + *                        It can be NULL, in which case an empty filter is assumed.
>> + *                        Upon successful return *out_bsf will contain the new, combined
>> + *                        bitstream filter which will be initialized.
>> + * @param[in]     bsf     the bitstream filter to be joined to the output of *out_bsf.
>> + *                        This filter must be uninitialized but it must be ready to be
>> + *                        initalized, so input codec parameters and time base must be
>> + *                        set if *out_bsf is NULL, otherwise the function will set these
>> + *                        automatically based on the output parameters of *out_bsf.
>> + *                        Upon successful return the bsf will be initialized.
>> + *
>> + * @return 0 on success, negative on error.
>
> One needs to be more explicit about what happens on error: bsf may be
> in a partially initialized state and is essentially only good for
> freeing (maybe the bsf parameter should be AVBSFContext **, too, and
> automatically free bsf on error?). But IMO we should aim to not cripple
> *out_bsf and document this.
>
>> + */
>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
>> +
>>  /**
>>   * Free a bitstream filter context and everything associated with it; write NULL
>>   * into the supplied pointer.
>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>> index b9fc771a88..1bca28d1ae 100644
>> --- a/libavcodec/bsf.c
>> +++ b/libavcodec/bsf.c
>> @@ -487,6 +487,68 @@ end:
>>      return ret;
>>  }
>> 
>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
>> +{
>> +    BSFListContext *lst;
>> +    AVBSFContext *obsf = *out_bsf;
>> +    AVBSFContext *new_list_bsf = NULL;
>> +    int ret;
>> +
>> +    if (!obsf) {
>> +        ret = av_bsf_init(bsf);
>> +        if (ret < 0)
>> +            return ret;
>> +        *out_bsf = bsf;
>> +        return 0;
>> +    }
>> +
>> +    if (obsf->filter != &ff_list_bsf) {
>> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
>> +        if (ret < 0)
>> +            return ret;
>> +        lst = new_list_bsf->priv_data;
>> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, obsf);
>> +        if (ret < 0)
>> +            goto fail;
>> +        ret = avcodec_parameters_copy(new_list_bsf->par_in, obsf->par_in);
>> +        if (ret < 0)
>> +            goto fail;
>> +        new_list_bsf->time_base_in = obsf->time_base_in;
>> +        obsf = new_list_bsf;
>> +    } else {
>> +        lst = obsf->priv_data;
>> +    }
>> +
>> +    ret = avcodec_parameters_copy(bsf->par_in, lst->bsfs[lst->nb_bsfs-1]->par_out);
>> +    if (ret < 0)
>> +        goto fail;
>> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
>> +
>> +    ret = av_bsf_init(&bsf);
>> +    if (ret < 0)
>> +        goto fail;
>> +
>> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
>
> This will change obsf even on failure (when *out_bsf was already of type
> ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
> currently doesn't do. I think we should leave obsf in a mostly unchanged
> state even if it implies an unnecessary allocation (it is really only a
> single allocation). This can be achieved by taking obsf's par_out in the
> else branch above and replacing it with fresh AVCodecParameters. On
> success, the old par_out is freed; on failure it is restored.
>
>> +    if (ret < 0)
>> +        goto fail;
>> +    obsf->time_base_out = bsf->time_base_out;
>> +
>> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
>> +    if (ret < 0)
>> +        goto fail;
>> +
>> +    *out_bsf = obsf;
>> +    return 0;
>> +
>> +fail:
>> +    if (new_list_bsf) {
>> +        if (lst->nb_bsfs)
>> +            lst->bsfs[0] = NULL;
>> +        av_bsf_free(&new_list_bsf);
>> +    }
>> +    return ret;
>> +}
>> +
>>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>>  {
>>      char *bsf_name, *bsf_options_str, *buf;
>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>> index f4d1d4de21..dadca75430 100644
>> --- a/libavcodec/version.h
>> +++ b/libavcodec/version.h
>> @@ -29,7 +29,7 @@
>>
>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>  #define LIBAVCODEC_VERSION_MINOR  77
>> -#define LIBAVCODEC_VERSION_MICRO 101
>> +#define LIBAVCODEC_VERSION_MICRO 102
>
> Adding a new public function always requires a minor bump.
>>
>>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>                                                 LIBAVCODEC_VERSION_MINOR, \
>> 
>
> Finally, (I feel bad having to tell you this given that you have clearly
> invested a lot of time in this) we currently have no case where we need
> more than one automatically inserted bitstream filter per stream. We do
> not know whether the current code does the right thing in case more than
> one packet needs to be analyzed to determine whether to insert bitstream
> filters (maybe the packets should instead be cached so that no bsf
> misses out on the initial packets?); of course what is the right thing
> might be format and codec-dependent. So I don't know whether we should
> add this function now (given that public API is hard to remove).
>
> This does not mean that we should not use a bsf_list bsf to simplify
> automatic bitstream filtering in mux.c (it really simplifies this alot
> and gets rid of code duplication). But I think we should leave the topic
> of adding a bsf to an already existing bsf open so that it can be
> determined when the need indeed arises.

I see no sane way of sharing list bsf code other than adding public API, 
because mux.c is libavformat, bsf.c is libavcodec. This at least allows 
you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs with 
a single bitstream filter to which you join additional ones.

Unless you want to remove the existing (admittedly unused, and potentially 
inadequate) support for multiple bitstream filters in AVStreamInternal, I 
really don't see a way which postpones adding this API.

Regards,
Marton