[FFmpeg-devel] [PATCH 2/2] avcodec/bsf: add av_bsf_join() to chain bitstream filters
Marton Balint
cus at passwd.hu
Fri Apr 10 11:25:02 EEST 2020
On Thu, 9 Apr 2020, James Almer wrote:
> 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?
That API only works if you have all the bitstream filters before
finalizing the list, you can't append a BSF to an already finalized
list. The implementation in mux.c supports adding a BSF, then maybe
passing a few packets through it, then appending another BSF to the list -
this can happen if .check_bitstream_filters() adds a bsf but returns 0,
and at a later packet it adds another bsf.
Regards,
Marton
More information about the ffmpeg-devel
mailing list