[FFmpeg-devel] [PATCH 2/2] avcodec/bsf: add av_bsf_join() to chain bitstream filters
James Almer
jamrial at gmail.com
Fri Apr 10 17:09:34 EEST 2020
On 4/10/2020 5:25 AM, Marton Balint wrote:
>
>
> 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
I see what you mean. Do we really need the option to insert a bsf to a
given stream after a few packets have already been processed? Any
required bsf should be present since the beginning. They are all written
to either modify the data or pass it through depending if something must
be done or not.
What could show up say five packets into the stream that would require a
bsf to be inserted at that point and couldn't have been present since
the beginning? Passing packets through within a bsf is almost free, so
there's not a lot to win by postponing inserting a bsf. You'll simply be
delegating the task of analyzing the bitstream to the muxer's
check_bitstream(), when the bsf can already do it without any extra steps.
More information about the ffmpeg-devel
mailing list