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

[Andreas Rheinhardt]

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.

- Andreas