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

[Marton Balint]

On Wed, 6 Mar 2024, Tomas Härdin wrote:

> lör 2024-02-24 klockan 15:13 +0100 skrev 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.
>
> D'oh! Then it's an entirely different thing of course. Then the onus
> falls on lavf internals to behave correctly.
>
>> 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.
>
> We could do that, or we could write an appropriate UL. Either is fine I
> suppose.

Will apply.

Thanks,
Marton