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

[Andreas Rheinhardt]

James Almer:
> On 4/10/2020 11:56 AM, Marton Balint wrote:
>>
>>
>> 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.
> 
> This will also allow you to simplify things by moving the
> oformat->check_bitstream() calls from do_packet_auto_bsf() to
> init_muxer(), right after the call to oformat->init(), since you're not
> longer constrained by the need for packets mid muxing.
> 
If it is decided that we no longer need to actually look at packet data
any more for adding bitstream filters, then we can actually remove the
check_bitstream functions altogether and let the muxer set things up
during init.

But this is not how I understood Marton. He only wanted to take away the
muxer's ability to add bitstream filters during multiple calls to
check_bitstream().

Btw: need_auto_bsf() from [1] can also be simplified a lot: One adds a
new field to AVStreamInternal bsf_checking_status. Possible values are 1
(checking is finished and one needs bsf), 0 (checking is finished and no
bsf are needed) and -1 (checking not finished). If the
AVFMT_FLAG_AUTO_BSF is not set or if the AVOutputFormat doesn't have a
check_bitstream() function, it is set to 0 during init, otherwise to -1.
need_auto_bsf() would then begin with "if
(st->internal->bsf_checking_status >= 0) return
st->internal->bsf_checking_status;". Otherwise, the bitstream would
really be checked (the check for the existence of the check_bitstream
function could then be omitted, too, of course), which would update
bsf_checking_status accordingly.

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/259204.html