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

[Andreas Rheinhardt]

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?).
> 
According to me: yes. But there is one little detail that I just
noticed: write_packets_common() has an AVStream parameter and both of
its callers (av_write_frame() and av_interleaved_write_frame()
essentially pass s->streams[pkt->stream_index] into it, but don't use it
themselves. This parameter should be eliminated and replaced by a local
variable in write_packets_common().

- Andreas