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

[Marton Balint]

On Fri, 10 Apr 2020, James Almer wrote:

> On 4/10/2020 5:25 AM, Marton Balint wrote:

[...]

>>>>>
>>>>> 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.

Ok, so this also points me to the direction of removing the support of 
multiple BSFs from AVStreamInternal, because if a demuxer wants multiple 
bsfs it can simply create a single list bsf in one go.

Is there anybody who objects removing support for multiple bsfs is 
AVStreamInternal?

Thanks,
Marton