[FFmpeg-devel] [PATCH v4 4/8] avformat/mux: add proper support for full N:M bitstream filtering

[Marton Balint]

On Tue, 5 May 2020, Marton Balint wrote:

>
>
> On Tue, 5 May 2020, Andreas Rheinhardt wrote:
>
>> Marton Balint:
>>> 
>>> 
>>> On Tue, 5 May 2020, Andreas Rheinhardt wrote:
>>> 
>>>> Marton Balint:
>>>>> Previously only 1:1 bitstream filters were supported, the end of the
>>>>> stream was
>>>>> not signalled to the bitstream filters and time base changes were
>>>>> ignored.
>>>>>
>>>>> This change also allows muxers to set up bitstream filters regardless
>>>>> of the
>>>>> autobsf flag during write_header instead of during check_bitstream
>>>>> and those
>>>>> bitstream filters will always be executed.
>>>>
>>>> Ignoring the autobsf flag is not ok IMO. The muxer should not add a bsf
>>>> when the user explicitly didn't want this.
>>>>
>>> 
>>> The user should not be allowed to create broken files, and the only way
>>> to achieve that is to force the BSF. I don't see a use case for
>>> disabling BSF either for MXFenc of GXFenc. (in fact, from the top of my
>>> head I can't see a use case for disabling automatically inserted BSF-s
>>> in other muxers).
>>> 
>> When automatic bitstream filtering was introduced in
>> 1f9139b07b8a896b62c1f28f3d04acac33978c0d, writing the header of every
>> muxer that potentially might include a bsf automatically (as indicated
>> by the existence of the check_bitstream()-function) was delayed until
>> the first packet would be written. This meant that there was a high
>> probability of having a packet for the relevant stream when using the
>> interleaved codepath.
>> In particular, this meant that the Matroska muxer now received proper
>> extradata for AAC when writing the header even before it received any
>> packet with side data containing potential extradata at all. The reason
>> was that the aac_adtstoasc bsf has simply overwritten the output
>> extradata when dealing with the first packet (that was the old bsf API
>> without init functions; when the new bsf API was merged, the merge
>> commit (not the original commit) propagated the API violating behaviour).
>> 1f6d7eb47070afc4394348721cd149f940ad2386 added the autobsf flag because
>> of this delay: After all, the delay existed as soon as the
>> AVOutputFormat had a check_bitstream function, even when no stream had a
>> codec for which the check_bitstream function would add a bsf at all.
>>
>> As time passed, the API-violating behaviour of aac_adtstoasc was fixed
>> and d6d605eb05c3ca32f591016c345eb2ad9e81c554 stopped delaying writing
>> the header. The very same patchset containing said commit also contained
>> a patch to deprecate AVFMT_FLAG_AUTO_BSF [1]. It was not applied due to
>> concerns what happens when the bsf is not available.
>>
>> (For the record, ff_stream_add_bitstream_filter() will error out if a
>> bsf is not available and the packet discarded. If the caller sends
>> another packet for this stream and a bsf that should be added isn't
>> available the packet will be rejected as well and so on.)
>>
>> The fact that one is forced to include certain automatically inserted
>> bitstream filters even when one knows that one will only use them in
>> scenarios where they merely passthrough the packets is certainly a
>> downside of eliminating this flag. But now that I have reread the
>> history I am nevertheless in favour of deprecation.
>
> Wow, thanks for checking this. But if we are moving in the direction of 
> deprecation, then I'd rather not add the extra check to 
> ff_stream_add_bitstream_filter because it will be removed later anyway.
>
> Also when I tried to add that, there was a fate failure caused by the 
> segment muxer setting the -autobsf flag but calling the muxer's 
> check_bitstream function directly. Or should I add create a patch which 
> simply removes the -autobsf additional options from segment.c?

So considering that autobsf will probably be deprecated anyway can I apply 
the patch as is? (And finally the series?).

Thanks,
Marton