diff mbox series

[FFmpeg-devel] avformat/mxfenc: Never set codec_ul UID to NULL

Message ID 20200228041432.4454-1-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/mxfenc: Never set codec_ul UID to NULL
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Feb. 28, 2020, 4:14 a.m. UTC
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(-)

Comments

Tomas Härdin March 2, 2020, 6:14 p.m. UTC | #1
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
Baptiste Coudurier March 2, 2020, 9:59 p.m. UTC | #2
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 mbox series

Patch

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)