diff mbox series

[FFmpeg-devel,01/17] avformat/mxfenc: Auto-insert h264_mp4toannexb BSF if needed

Message ID AM7PR03MB66600B2742E305A97C277D898F929@AM7PR03MB6660.eurprd03.prod.outlook.com
State New
Headers
Series [FFmpeg-devel,01/17] avformat/mxfenc: Auto-insert h264_mp4toannexb BSF if needed |

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Nov. 9, 2021, 5:34 p.m. UTC 
The mxf and mxf_opatom muxer expect H.264 in Annex B format.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
The check here is taken from mpegtsenc.

 libavformat/mxfenc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

James Almer Nov. 9, 2021, 6:10 p.m. UTC | #1 
On 11/9/2021 2:34 PM, Andreas Rheinhardt wrote:
> The mxf and mxf_opatom muxer expect H.264 in Annex B format.

Amazing that nobody noticed this until now. Guess remuxing from mp4/mkv 
to mxf is not a common scenario.

LGTM, just tested a quick remux and without the bsf it just fails.

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> The check here is taken from mpegtsenc.
> 
>   libavformat/mxfenc.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index c36ebef932..d1c4d43a50 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -3174,6 +3174,22 @@ static int mxf_interleave(AVFormatContext *s, AVPacket *pkt,
>       return mxf_interleave_get_packet(s, pkt, flush);
>   }
>   
> +static int mxf_check_bitstream(AVFormatContext *s, const AVPacket *pkt)
> +{
> +    AVStream *const st = s->streams[pkt->stream_index];
> +
> +    switch (st->codecpar->codec_id) {
> +    case AV_CODEC_ID_H264:
> +        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
> +                             (AV_RB24(pkt->data) != 0x000001 ||
> +                              (st->codecpar->extradata_size > 0 &&
> +                               st->codecpar->extradata[0] == 1)))
> +            return ff_stream_add_bitstream_filter(st, "h264_mp4toannexb", NULL);
> +        break;
> +    }
> +    return 1;
> +}
> +
>   #define MXF_COMMON_OPTIONS \
>       { "signal_standard", "Force/set Signal Standard",\
>         offsetof(MXFContext, signal_standard), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 7, AV_OPT_FLAG_ENCODING_PARAM, "signal_standard"},\
> @@ -3249,6 +3265,7 @@ const AVOutputFormat ff_mxf_muxer = {
>       .audio_codec       = AV_CODEC_ID_PCM_S16LE,
>       .video_codec       = AV_CODEC_ID_MPEG2VIDEO,
>       .write_header      = mxf_write_header,
> +    .check_bitstream   = mxf_check_bitstream,
>       .write_packet      = mxf_write_packet,
>       .write_trailer     = mxf_write_footer,
>       .deinit            = mxf_deinit,
> @@ -3282,6 +3299,7 @@ const AVOutputFormat ff_mxf_opatom_muxer = {
>       .audio_codec       = AV_CODEC_ID_PCM_S16LE,
>       .video_codec       = AV_CODEC_ID_DNXHD,
>       .write_header      = mxf_write_header,
> +    .check_bitstream   = mxf_check_bitstream,
>       .write_packet      = mxf_write_packet,
>       .write_trailer     = mxf_write_footer,
>       .deinit            = mxf_deinit,
>
Tomas Härdin Nov. 9, 2021, 9 p.m. UTC | #2 
tis 2021-11-09 klockan 18:34 +0100 skrev Andreas Rheinhardt:
> The mxf and mxf_opatom muxer expect H.264 in Annex B format.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> The check here is taken from mpegtsenc.

You didn't think to make both muxers share code instead of copy-
pasting?

/Tomas
Andreas Rheinhardt Nov. 9, 2021, 9:07 p.m. UTC | #3 
Tomas Härdin:
> tis 2021-11-09 klockan 18:34 +0100 skrev Andreas Rheinhardt:
>> The mxf and mxf_opatom muxer expect H.264 in Annex B format.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> The check here is taken from mpegtsenc.
> 
> You didn't think to make both muxers share code instead of copy-
> pasting?
> 

Well, I can share it. The problem is just that I didn't really know
where it belongs: mux.c? utils.c? A new file?

- Andreas
Tomas Härdin Nov. 9, 2021, 10:38 p.m. UTC | #4 
tis 2021-11-09 klockan 22:07 +0100 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2021-11-09 klockan 18:34 +0100 skrev Andreas Rheinhardt:
> > > The mxf and mxf_opatom muxer expect H.264 in Annex B format.
> > > 
> > > Signed-off-by: Andreas Rheinhardt
> > > <andreas.rheinhardt@outlook.com>
> > > ---
> > > The check here is taken from mpegtsenc.
> > 
> > You didn't think to make both muxers share code instead of copy-
> > pasting?
> > 
> 
> Well, I can share it. The problem is just that I didn't really know
> where it belongs: mux.c? utils.c? A new file?

A new file probably, say libavformat/annexb.c

Do we ever need to be able to do this kind of stuff in lavc? In that
case it could maybe live there. But that again increases coupling
between the two libs..

/Tomas
Andreas Rheinhardt Nov. 10, 2021, 1:21 p.m. UTC | #5 
Tomas Härdin:
> tis 2021-11-09 klockan 22:07 +0100 skrev Andreas Rheinhardt:
>> Tomas Härdin:
>>> tis 2021-11-09 klockan 18:34 +0100 skrev Andreas Rheinhardt:
>>>> The mxf and mxf_opatom muxer expect H.264 in Annex B format.
>>>>
>>>> Signed-off-by: Andreas Rheinhardt
>>>> <andreas.rheinhardt@outlook.com>
>>>> ---
>>>> The check here is taken from mpegtsenc.
>>>
>>> You didn't think to make both muxers share code instead of copy-
>>> pasting?
>>>
>>
>> Well, I can share it. The problem is just that I didn't really know
>> where it belongs: mux.c? utils.c? A new file?
> 
> A new file probably, say libavformat/annexb.c
> 
> Do we ever need to be able to do this kind of stuff in lavc? In that
> case it could maybe live there. But that again increases coupling
> between the two libs..
> 

lavc does not have AVStreams or AVFormatContexts, so sharing code would
be absolutely impossible anyway. But one can share ideas: Using a bsf to
prepare data for a decoder is also done for certain decoders; see
AVCodec.bsfs. These bitstream filters are applied unconditionally and
are therefore supposed to detect themselves whether they should do
something or whether the data already has the desired form. I am unsure
whether this is a better approach; doing the same in lavf would add a
problem that lavc does not have: There would be a copy of every
non-refcounted packet when using av_write_frame().

- Andreas
Tomas Härdin Nov. 10, 2021, 3:13 p.m. UTC | #6 
ons 2021-11-10 klockan 14:21 +0100 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2021-11-09 klockan 22:07 +0100 skrev Andreas Rheinhardt:
> > > Tomas Härdin:
> > > > tis 2021-11-09 klockan 18:34 +0100 skrev Andreas Rheinhardt:
> > > > > The mxf and mxf_opatom muxer expect H.264 in Annex B format.
> > > > > 
> > > > > Signed-off-by: Andreas Rheinhardt
> > > > > <andreas.rheinhardt@outlook.com>
> > > > > ---
> > > > > The check here is taken from mpegtsenc.
> > > > 
> > > > You didn't think to make both muxers share code instead of
> > > > copy-
> > > > pasting?
> > > > 
> > > 
> > > Well, I can share it. The problem is just that I didn't really
> > > know
> > > where it belongs: mux.c? utils.c? A new file?
> > 
> > A new file probably, say libavformat/annexb.c
> > 
> > Do we ever need to be able to do this kind of stuff in lavc? In
> > that
> > case it could maybe live there. But that again increases coupling
> > between the two libs..
> > 
> 
> lavc does not have AVStreams or AVFormatContexts, so sharing code
> would
> be absolutely impossible anyway. But one can share ideas: Using a bsf
> to
> prepare data for a decoder is also done for certain decoders; see
> AVCodec.bsfs. These bitstream filters are applied unconditionally and
> are therefore supposed to detect themselves whether they should do
> something or whether the data already has the desired form. I am
> unsure
> whether this is a better approach; doing the same in lavf would add a
> problem that lavc does not have: There would be a copy of every
> non-refcounted packet when using av_write_frame().

Just noting here that it might be that we shouldn't even insert a BSF.
See my comment on patch 03.

/Tomas
diff mbox series

Patch

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index c36ebef932..d1c4d43a50 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -3174,6 +3174,22 @@  static int mxf_interleave(AVFormatContext *s, AVPacket *pkt,
     return mxf_interleave_get_packet(s, pkt, flush);
 }
 
+static int mxf_check_bitstream(AVFormatContext *s, const AVPacket *pkt)
+{
+    AVStream *const st = s->streams[pkt->stream_index];
+
+    switch (st->codecpar->codec_id) {
+    case AV_CODEC_ID_H264:
+        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
+                             (AV_RB24(pkt->data) != 0x000001 ||
+                              (st->codecpar->extradata_size > 0 &&
+                               st->codecpar->extradata[0] == 1)))
+            return ff_stream_add_bitstream_filter(st, "h264_mp4toannexb", NULL);
+        break;
+    }
+    return 1;
+}
+
 #define MXF_COMMON_OPTIONS \
     { "signal_standard", "Force/set Signal Standard",\
       offsetof(MXFContext, signal_standard), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 7, AV_OPT_FLAG_ENCODING_PARAM, "signal_standard"},\
@@ -3249,6 +3265,7 @@  const AVOutputFormat ff_mxf_muxer = {
     .audio_codec       = AV_CODEC_ID_PCM_S16LE,
     .video_codec       = AV_CODEC_ID_MPEG2VIDEO,
     .write_header      = mxf_write_header,
+    .check_bitstream   = mxf_check_bitstream,
     .write_packet      = mxf_write_packet,
     .write_trailer     = mxf_write_footer,
     .deinit            = mxf_deinit,
@@ -3282,6 +3299,7 @@  const AVOutputFormat ff_mxf_opatom_muxer = {
     .audio_codec       = AV_CODEC_ID_PCM_S16LE,
     .video_codec       = AV_CODEC_ID_DNXHD,
     .write_header      = mxf_write_header,
+    .check_bitstream   = mxf_check_bitstream,
     .write_packet      = mxf_write_packet,
     .write_trailer     = mxf_write_footer,
     .deinit            = mxf_deinit,