Message ID | 20200228041432.4454-1-andreas.rheinhardt@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel] avformat/mxfenc: Never set codec_ul UID to NULL | expand |
Context | Check | Description |
---|---|---|
andriy/ffmpeg-patchwork | success | Make fate finished |
fre 2020-02-28 klockan 05:14 +0100 skrev Andreas Rheinhardt: > mxf distinguishes codec profiles by different UIDs and therefore needs > to check that the input is actually compatible with mxf (i.e. if there > is a defined UID for it). If not, then sometimes the UID would be set to > NULL and writing the (video) packet would fail. Yet the following audio > packet would trigger writing the header (which has been postponed because > the UID is not known at the start) and if the UID is NULL, this can lead > to segfaults. This commit therefore stops setting the UID to NULL if the > input is incompatible with mxf (it has initially been set to a generic > value in mxf_write_header()). > > Fixes #7993. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > a) #7993 is actually a segfault even if its description suggests > otherwise. > b) My actually preferred way to fix this was to error out if an audio > packet is encountered and the header has not been written yet, thereby > ensuring that only a video packet can trigger writing the header and > said code is only reached when the video is actually compatible with mxf > (or with this muxer?). Yet I was surprised to find out that the > FATE-test mxf-d10-user-comments would fail with this, because right now > writing the header in this test is triggered by an audio packet (the > video packets aren't written because they are not of the right size) and > if these aren't written either, there will be zero bytes of output. > c) It seems that mxf_write_header() doesn't actually write anything and > could be converted into an init function. > > libavformat/mxfenc.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) Looks OK. Maybe we should have some av_asserts in the remaining places where sc->codec_ul is used? /Tomas
Hi Andreas, > On Mar 2, 2020, at 10:14 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote: > > fre 2020-02-28 klockan 05:14 +0100 skrev Andreas Rheinhardt: >> mxf distinguishes codec profiles by different UIDs and therefore needs >> to check that the input is actually compatible with mxf (i.e. if there >> is a defined UID for it). If not, then sometimes the UID would be set to >> NULL and writing the (video) packet would fail. Yet the following audio >> packet would trigger writing the header (which has been postponed because >> the UID is not known at the start) and if the UID is NULL, this can lead >> to segfaults. This commit therefore stops setting the UID to NULL if the >> input is incompatible with mxf (it has initially been set to a generic >> value in mxf_write_header()). >> >> Fixes #7993. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> a) #7993 is actually a segfault even if its description suggests >> otherwise. >> b) My actually preferred way to fix this was to error out if an audio >> packet is encountered and the header has not been written yet, thereby >> ensuring that only a video packet can trigger writing the header and >> said code is only reached when the video is actually compatible with mxf >> (or with this muxer?). Yet I was surprised to find out that the >> FATE-test mxf-d10-user-comments would fail with this, because right now >> writing the header in this test is triggered by an audio packet (the >> video packets aren't written because they are not of the right size) and >> if these aren't written either, there will be zero bytes of output. Yes, we need the video track before writing the header. It’s an issue and that’s what should be implemented IMHO. — Baptiste
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index 7ea47d7311..c00ab9d1c3 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -1952,7 +1952,6 @@ static int mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket *pk if (mxf->header_written) return 1; - sc->codec_ul = NULL; profile = st->codecpar->profile; for (i = 0; i < FF_ARRAY_ELEMS(mxf_prores_codec_uls); i++) { if (profile == mxf_prores_codec_uls[i].profile) { @@ -1960,7 +1959,7 @@ static int mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket *pk break; } } - if (!sc->codec_ul) + if (i == FF_ARRAY_ELEMS(mxf_prores_codec_uls)) return 0; sc->frame_size = pkt->size; @@ -2006,7 +2005,6 @@ static int mxf_parse_dnxhd_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt if (pkt->size < 43) return 0; - sc->codec_ul = NULL; cid = AV_RB32(pkt->data + 0x28); for (i = 0; i < FF_ARRAY_ELEMS(mxf_dnxhd_codec_uls); i++) { if (cid == mxf_dnxhd_codec_uls[i].cid) { @@ -2014,7 +2012,7 @@ static int mxf_parse_dnxhd_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt break; } } - if (!sc->codec_ul) + if (i == FF_ARRAY_ELEMS(mxf_dnxhd_codec_uls)) return 0; sc->component_depth = 0; @@ -2177,6 +2175,7 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st, const uint8_t *buf = pkt->data; const uint8_t *buf_end = pkt->data + pkt->size; const uint8_t *nal_end; + const UID *codec_ul = NULL; uint32_t state = -1; int extra_size = 512; // support AVC Intra files without SPS/PPS header int i, frame_size, slice_type, intra_only = 0; @@ -2246,12 +2245,11 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st, if (!sps) sc->interlaced = st->codecpar->field_order != AV_FIELD_PROGRESSIVE ? 1 : 0; - sc->codec_ul = NULL; frame_size = pkt->size + extra_size; for (i = 0; i < FF_ARRAY_ELEMS(mxf_h264_codec_uls); i++) { if (frame_size == mxf_h264_codec_uls[i].frame_size && sc->interlaced == mxf_h264_codec_uls[i].interlaced) { - sc->codec_ul = &mxf_h264_codec_uls[i].uid; + codec_ul = &mxf_h264_codec_uls[i].uid; sc->component_depth = 10; // AVC Intra is always 10 Bit sc->aspect_ratio = (AVRational){ 16, 9 }; // 16:9 is mandatory for broadcast HD st->codecpar->profile = mxf_h264_codec_uls[i].profile; @@ -2265,7 +2263,7 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st, mxf_h264_codec_uls[i].profile == sps->profile_idc && (mxf_h264_codec_uls[i].intra_only < 0 || mxf_h264_codec_uls[i].intra_only == intra_only)) { - sc->codec_ul = &mxf_h264_codec_uls[i].uid; + codec_ul = &mxf_h264_codec_uls[i].uid; st->codecpar->profile = sps->profile_idc; st->codecpar->level = sps->level_idc; // continue to check for avc intra @@ -2274,10 +2272,11 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st, av_free(sps); - if (!sc->codec_ul) { + if (!codec_ul) { av_log(s, AV_LOG_ERROR, "h264 profile not supported\n"); return 0; } + sc->codec_ul = codec_ul; return 1; } @@ -2374,9 +2373,13 @@ static int mxf_parse_mpeg2_frame(AVFormatContext *s, AVStream *st, } } } - if (s->oformat != &ff_mxf_d10_muxer) - sc->codec_ul = mxf_get_mpeg2_codec_ul(st->codecpar); - return !!sc->codec_ul; + if (s->oformat != &ff_mxf_d10_muxer) { + const UID *codec_ul = mxf_get_mpeg2_codec_ul(st->codecpar); + if (!codec_ul) + return 0; + sc->codec_ul = codec_ul; + } + return 1; } static uint64_t mxf_parse_timestamp(int64_t timestamp64)
mxf distinguishes codec profiles by different UIDs and therefore needs to check that the input is actually compatible with mxf (i.e. if there is a defined UID for it). If not, then sometimes the UID would be set to NULL and writing the (video) packet would fail. Yet the following audio packet would trigger writing the header (which has been postponed because the UID is not known at the start) and if the UID is NULL, this can lead to segfaults. This commit therefore stops setting the UID to NULL if the input is incompatible with mxf (it has initially been set to a generic value in mxf_write_header()). Fixes #7993. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- a) #7993 is actually a segfault even if its description suggests otherwise. b) My actually preferred way to fix this was to error out if an audio packet is encountered and the header has not been written yet, thereby ensuring that only a video packet can trigger writing the header and said code is only reached when the video is actually compatible with mxf (or with this muxer?). Yet I was surprised to find out that the FATE-test mxf-d10-user-comments would fail with this, because right now writing the header in this test is triggered by an audio packet (the video packets aren't written because they are not of the right size) and if these aren't written either, there will be zero bytes of output. c) It seems that mxf_write_header() doesn't actually write anything and could be converted into an init function. libavformat/mxfenc.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)