[FFmpeg-devel] [PATCH 2/2] avcodec/bsf: add av_bsf_join() to chain bitstream filters
Marton Balint
cus at passwd.hu
Fri Apr 10 14:52:22 EEST 2020
On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
> Marton Balint:
>>
>>
>> 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.
>>>
> When I wrote the above, I thought that we might restrict the code in
> mux.c to a single bsf by use of the list bsf and that we postpone the
> exact details of how to add a bsf to an already existing list to the
> time when it is really needed; in particular, we should not strive to
> mimic what the current code does in this case because we do not know
> whether this is indeed the right thing.
>
> Thinking about this a bit further has made me realize that there is
> another problem: It might be that a muxer might need the output of the
> first bitstream filter in order to determine whether to add another
> bitstream filter to the bsf chain. This is a usecase that can't be
> fulfilled with av_bsf_join() and I think it can't be fulfilled with a
> single list-bsf at all.
> In this case, one would essentially put what you have put into
> need_auto_bsf() into the loop to be called at the end of the current
> filter chain* and if it resulted in a new filter being appended, the
> packet would of course need to be sent to said new filter.
> This scenario can definitely not be solved with av_bsf_join() (it's not
> designed for partially filtered packets). And in this case the code in
> mux.c will have to deal with multiple bsfs anyway (the old one as well
> as the new one), so that no simplifications from using a single bsf will
> be achievable.
>
> So my current stance is: Keep support for manual bsf lists in mux.c, but
> leave the details of how to actually add multiple bsf to the future: The
> only supported case right now is adding multiple bsf in the same
> check_packet() call and this should be kept, but there is no reason to
> already move need_auto_bsf() into the loop. Fixing this is not realistic
> right now because we don't have a real usecase. And furthermore, it
> would delay your patchset even further.
> It also means that the first patch of this patchset should be applied,
> but not the second.
But how do you share code then?
Also I don't follow your logic, if you say that no new API should be added
until it is used, then why not remove the unused multiple bsf lists from
AVStreamInternal and re-add it when it is actually needed? That is the
only way I see to not add duplicated list code.
Regards,
Marton
More information about the ffmpeg-devel
mailing list