Marton Balint:
>
>
> 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 <c...@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.
>
I reconsidered again. My earlier objection was based on the belief that
there is no way to add an already initialized filter to a bsf(-list).
But there actually is: One just has to set the list's idx so that it
tries to drain the new filter first. This is safe.
So if a muxer ever needs the output of earlier filters to decide whether
to append new filters, this could be solved using the bsf-list API as
follows: check_bitstream() initializes the new bsf and already sends the
packet it received to the new bsf. Then it adds the new bsf to the old
bsf(-list) (this will have to be done in some form of av_bsf_join() and
said function would have to set the internal idx of the list to make it
drain the new bsf first; if the new bsf doesn't output anything yet, it
will automatically try the earlier filters again). Of course, not every
check_bitstream() function would have to do this itself as this part can
be factored out into a common function, just like adding a bsf is now
factored out into ff_stream_add_bitstream_filter().
(It should go without saying that check_bitstream() would no longer
receive a const AVPacket * and that the naming would also have to be
adapted if the above would be implemented.)
So I am now confident that we can do anything that a manual bsf list can
do with the bsf-list API as well.* So I see no reason any more to keep
bsf-lists in mux.c; it only needs to support a single bsf.
But given that we don't have a usecase for multiple automatically
inserted bsf at all at the moment, I also don't see a reason why we
should add av_bsf_join() now.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
