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

[Marton Balint]

On Thu, 7 May 2020, Andreas Rheinhardt wrote:

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

Thanks, applied the series with that fix.

Regards,
Marton