[FFmpeg-devel] [PATCH] avformat/mxfenc: add h264_mp4toannexb bitstream filter if needed when muxing h264

[Marton Balint]

On Sat, 24 Feb 2024, Andreas Rheinhardt wrote:

> Tomas Härdin:
>>>>> +static int mxf_check_bitstream(AVFormatContext *s, AVStream *st,
>>>>> const AVPacket *pkt)
>>>>> +{
>>>>> +    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
>>>>> +        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
>>>>> +                              AV_RB24(pkt->data) != 0x000001)
>>>>> +            return ff_stream_add_bitstream_filter(st,
>>>>> "h264_mp4toannexb", NULL);
>> 
>> Regardless of the comments below, this is wrong. ST 381-3 says this:
>> 
>>> The byte stream format can be constructed from the NAL unit stream by
>>> prefixing each NAL unit with a start
>>> code prefix and zero or more zero-valued bytes to form a stream of
>>> bytes.
>> 
>> Note the wording is "zero or more", not "zero or one".
>
> IMO all the code should only look at extradata to decide whether a
> stream is annex B or ISOBMFF (no extradata->annex B, no ISOBMFF
> extradata->annex B, else ISOBMFF). But that is a separate issue.
> (There is a slight possibility of misdetection here: E.g. a 0x00 00 01
> at the start of a packet can actually be the start of the length code of
> an ISOBMFF NALU with length in the range 256-511; on the other hand, it
> is legal for an annex B packet to start with four or more zero bytes, as
> you mentioned.)
>
>> The correct way to do this is to inspect byte 14 of the EC UL, per
>> section 8.1 of ST 381-3.
>
> This is a patch for the muxer, not the demuxer. There is no byte 14 of
> the EC UL to inspect; or at least: It is what this muxer writes for it.
> This muxer always indicates that the output is an annex B (aka AVC byte
> stream), so it should always convert the input from the user to actually
> be annex B.
>
>>>> I sent the very same patch long ago [1]. Tomas Härdin opposed it
>>>> [2],
>>>> [3], because he sees stuff like this as hack.
>> 
>> No, I oppose it because it is potentially against spec. The MXF
>> ecosystem is bad enough as it is without us encouraging out-of-spec
>> behavior.
>
> If the user's input is ISOBMFF, then the output will definitely be
> against spec without a conversion. With a patch like this ISOBMFF data
> will be converted to annex B.
>
> Anyway, FFmpeg aims to support two framings for H.264: Annex B and
> ISOBMFF. Sending ISOBMFF-framed data to a muxer is therefore not
> "out-of-spec behavior". It is just supposed to work and the onus is on
> the muxer/libavformat to convert as necessary.
>
> Also note that other missing checks for whether the input is really
> conforming to the specs should be separate from inserting this BSF.
> After all, the user could insert the BSF himself and even in this case
> it would be this muxer's responsibility to ensure that the output is
> spec-compliant. Inserting the BSF simplifies this task, because it means
> that the muxer can assume that the input is already annex B (and does
> not need separate logic for handling ISOBMFF input); that is the only
> point of inserting it.
>
>> Any behavior we put in to handle out-of-spec behavior should be limited
>> by Identification. But even that would be making our responsibility
>> what is really the responsibility of companies making broken MXF
>> muxers.
>
> Once again: This is a muxer, we do not parse and identify a file here.
> For the same reason it makes no sense to complain about other companies'
> broken MXF muxers.

I agree with what you wrote, so IMHO we should apply this, unless Tomas 
still has an objection.

Thanks,
Marton