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

2020-03-02 Thread Baptiste Coudurier

Hi Andreas,

> On Mar 2, 2020, at 10:14 AM, Tomas Härdin  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 
>> ---
>> 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


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2020-03-02 Thread Tomas Härdin
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 
> ---
> 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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2020-02-27 Thread 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 
---
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(-)

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