| Message ID | AM7PR03MB66600B2742E305A97C277D898F929@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/17] avformat/mxfenc: Auto-insert h264_mp4toannexb BSF if needed | expand |
| 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 |
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, >
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
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
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
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
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 --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,
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(+)