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

James Almer jamrial at gmail.com
Fri Apr 10 03:34:08 EEST 2020


On 4/9/2020 9:11 PM, Marton Balint wrote:
> 
> 
> 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.

Can't you achieve the same using existing bsf list API from within lavf?
av_bsf_list_alloc(), followed by any required amount of calls to
av_bsf_list_append(), av_bsf_list_append2() or av_bsf_list_parse_str()
to add each bsf, then av_bsf_list_finalize() at the end?


More information about the ffmpeg-devel mailing list