| Message ID | 20201204234806.1509656-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mxfenc: Discard audio until valid video has been received | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | fail | Make fate fail |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | fail | Make fate fail |
On Sat, 5 Dec 2020, Andreas Rheinhardt wrote: > Normally, video packets are muxed before audio packets for mxf (there is > a dedicated interleave function for this); furthermore the first (video) > packet triggers writing the actual header. Yet when the first video packet > fails the checks performed on it, codec_ul (a value set based upon > properties of the bitstream which necessitates actually inspecting > packets) may be NULL; the next packet written (an audio packet) will then > trigger outputting the header and this leads to a segfault because > codec_ul is dereferenced (this is ticket #7993). Therefore this commit > discards audio packets until a valid video packet has been received, > fixing said ticket. I don't think in general it is expected by muxers to do anything useful if a write_packet call fails and a subsequent write_packet call is attempted. A write_packet call error is not an error a muxer can recover from, and attempting another write_packet seems like undefined behaviour to me, admittedly this is not really documented. In the ticket above, it is write_trailer that is crashing because write_trailer tries to output more packets after an earlier write_packet failure. So I wonder if it would make more sense to change write trailer to not try to flush packets at all, because the muxer is already in a failed state, and only cleanup (calling ->write_trailer()) is a sane thing to do, not writing packets. Regards, Marton > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > This implements what I had originally in mind to fix said ticket and it > is also what Baptiste Coudurier wanted [1]. The main obstacle in fixing > it was fixing the mxf-d10-user-comments test (with this patch, the old > test would output nothing at all). > > [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/257900.html > > libavformat/mxfenc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c > index d8678c9d25..2b0b7342a7 100644 > --- a/libavformat/mxfenc.c > +++ b/libavformat/mxfenc.c > @@ -2843,6 +2843,13 @@ static int mxf_write_packet(AVFormatContext *s, AVPacket *pkt) > MXFIndexEntry ie = {0}; > int err; > > + if (!mxf->header_written && pkt->stream_index != 0 && > + s->oformat != &ff_mxf_opatom_muxer) { > + av_log(s, AV_LOG_ERROR, "Received non-video packet before " > + "header has been written\n"); > + return AVERROR_INVALIDDATA; > + } > + > if (!mxf->cbr_index && !mxf->edit_unit_byte_count && !(mxf->edit_units_count % EDIT_UNITS_PER_BODY)) { > if ((err = av_reallocp_array(&mxf->index_entries, mxf->edit_units_count > + EDIT_UNITS_PER_BODY, sizeof(*mxf->index_entries))) < 0) { > -- > 2.25.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint: > > > On Sat, 5 Dec 2020, Andreas Rheinhardt wrote: > >> Normally, video packets are muxed before audio packets for mxf (there is >> a dedicated interleave function for this); furthermore the first (video) >> packet triggers writing the actual header. Yet when the first video >> packet >> fails the checks performed on it, codec_ul (a value set based upon >> properties of the bitstream which necessitates actually inspecting >> packets) may be NULL; the next packet written (an audio packet) will then >> trigger outputting the header and this leads to a segfault because >> codec_ul is dereferenced (this is ticket #7993). Therefore this commit >> discards audio packets until a valid video packet has been received, >> fixing said ticket. > > > I don't think in general it is expected by muxers to do anything useful > if a write_packet call fails and a subsequent write_packet call is > attempted. > A write_packet call error is not an error a muxer can recover from, and > attempting another write_packet seems like undefined behaviour to me, > admittedly this is not really documented. > While this is true in lots of error scenarios, it is not true for all of them. It can also be that it is just a single packet that is crap and gets rejected; e.g. the Matroska muxer rejects packets without PTS (i.e. pkt->pts == AV_NO_PTS_VALUE), yet packets after that can be handled just fine. Another case are some output devices that return AVERROR(EAGAIN). And given that no prohibition is documented, adding such a prohibition would be an API break that could would require a deprecation period. But I am certainly not against a more generic solution. How about a new field in AVFormatInternal that a muxer can set if writing more packets is hopeless and should not be attempted any more and if it is set and a new attempt at writing a packet is attempted, it gets rejected without reaching the muxer at all. > In the ticket above, it is write_trailer that is crashing because > write_trailer tries to output more packets after an earlier write_packet > failure. Sure about that? My stacktrace says that it comes from av_interleaved_write_frame(); av_write_trailer() is never called. So I wonder if it would make more sense to change write trailer > to not try to flush packets at all, because the muxer is already in a > failed state, and only cleanup (calling ->write_trailer()) is a sane > thing to do, not writing packets. > > Regards, > Marton > >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> This implements what I had originally in mind to fix said ticket and it >> is also what Baptiste Coudurier wanted [1]. The main obstacle in fixing >> it was fixing the mxf-d10-user-comments test (with this patch, the old >> test would output nothing at all). >> >> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/257900.html >> >> libavformat/mxfenc.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c >> index d8678c9d25..2b0b7342a7 100644 >> --- a/libavformat/mxfenc.c >> +++ b/libavformat/mxfenc.c >> @@ -2843,6 +2843,13 @@ static int mxf_write_packet(AVFormatContext *s, >> AVPacket *pkt) >> MXFIndexEntry ie = {0}; >> int err; >> >> + if (!mxf->header_written && pkt->stream_index != 0 && >> + s->oformat != &ff_mxf_opatom_muxer) { >> + av_log(s, AV_LOG_ERROR, "Received non-video packet before " >> + "header has been written\n"); >> + return AVERROR_INVALIDDATA; >> + } >> + >> if (!mxf->cbr_index && !mxf->edit_unit_byte_count && >> !(mxf->edit_units_count % EDIT_UNITS_PER_BODY)) { >> if ((err = av_reallocp_array(&mxf->index_entries, >> mxf->edit_units_count >> + EDIT_UNITS_PER_BODY, >> sizeof(*mxf->index_entries))) < 0) { >> -- >> 2.25.1 >>
On Sun, 6 Dec 2020, Andreas Rheinhardt wrote: > Marton Balint: >> >> >> On Sat, 5 Dec 2020, Andreas Rheinhardt wrote: >> >>> Normally, video packets are muxed before audio packets for mxf (there is >>> a dedicated interleave function for this); furthermore the first (video) >>> packet triggers writing the actual header. Yet when the first video >>> packet >>> fails the checks performed on it, codec_ul (a value set based upon >>> properties of the bitstream which necessitates actually inspecting >>> packets) may be NULL; the next packet written (an audio packet) will then >>> trigger outputting the header and this leads to a segfault because >>> codec_ul is dereferenced (this is ticket #7993). Therefore this commit >>> discards audio packets until a valid video packet has been received, >>> fixing said ticket. >> >> >> I don't think in general it is expected by muxers to do anything useful >> if a write_packet call fails and a subsequent write_packet call is >> attempted. >> A write_packet call error is not an error a muxer can recover from, and >> attempting another write_packet seems like undefined behaviour to me, >> admittedly this is not really documented. >> > While this is true in lots of error scenarios, it is not true for all of > them. It can also be that it is just a single packet that is crap and > gets rejected; e.g. the Matroska muxer rejects packets without PTS (i.e. > pkt->pts == AV_NO_PTS_VALUE), yet packets after that can be handled just > fine. This looks like something that happens to work this way, not intentionally. And how broken the file will be for which the muxer rejected some packets? > Another case are some output devices that return AVERROR(EAGAIN). Good point, EAGAIN should not be considered fatal. Although EAGAIN is kind of an exception, because that means that you should wait some time and retry the same packet later, and not continue with the next packet... > And given that no prohibition is documented, adding such a prohibition > would be an API break that could would require a deprecation period. I think you overestimate the usefulness of this feature. Sure, we can wait the API bump with this change, but I don't think it's worth waiting for a deprecation period. Anyhow, since general solution clearly won't happen anytime soon, feel free to apply the patch as is. > > But I am certainly not against a more generic solution. How about a new > field in AVFormatInternal that a muxer can set if writing more packets > is hopeless and should not be attempted any more and if it is set and a > new attempt at writing a packet is attempted, it gets rejected without > reaching the muxer at all. This should be opt-out, not opt-in IMHO, because not counting EAGAIN I am still skeptical about a particularly useful scenario. Maybe a muxer flag is enough? Although before adding any support for this, I still would like to see a justified use. > >> In the ticket above, it is write_trailer that is crashing because >> write_trailer tries to output more packets after an earlier write_packet >> failure. > > Sure about that? My stacktrace says that it comes from > av_interleaved_write_frame(); av_write_trailer() is never called. I tested the command line on the ticket: ffmpeg_g -y -i tmp.vob -map 0 -c:v prores_ks -c:v:122 fits -disposition:a:39 h261 -disposition:a:114 wmv1 -vframes 17 -b:v 587 -strict 1 tmp_.mxf and got this with gdb at the crash: __memcpy_ssse3 at /lib64/libc.so.6 avio_write at libavformat/aviobuf.c:234 mxf_write_cdci_common at libavformat/mxfenc.c:1244 mxf_write_cdci_desc at libavformat/mxfenc.c:1324 mxf_write_package at libavformat/mxfenc.c:1612 mxf_write_header_metadata_sets at libavformat/mxfenc.c:1683 mxf_write_partition at libavformat/mxfenc.c:1944 mxf_write_packet at libavformat/mxfenc.c:2903 write_packet at libavformat/mux.c:731 interleaved_write_packet at libavformat/mux.c:1104 av_write_trailer at libavformat/mux.c:1266 transcode at fftools/ffmpeg.c:4795 main at fftools/ffmpeg.c:4966 Regards, Marton
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index d8678c9d25..2b0b7342a7 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -2843,6 +2843,13 @@ static int mxf_write_packet(AVFormatContext *s, AVPacket *pkt) MXFIndexEntry ie = {0}; int err; + if (!mxf->header_written && pkt->stream_index != 0 && + s->oformat != &ff_mxf_opatom_muxer) { + av_log(s, AV_LOG_ERROR, "Received non-video packet before " + "header has been written\n"); + return AVERROR_INVALIDDATA; + } + if (!mxf->cbr_index && !mxf->edit_unit_byte_count && !(mxf->edit_units_count % EDIT_UNITS_PER_BODY)) { if ((err = av_reallocp_array(&mxf->index_entries, mxf->edit_units_count + EDIT_UNITS_PER_BODY, sizeof(*mxf->index_entries))) < 0) {
Normally, video packets are muxed before audio packets for mxf (there is a dedicated interleave function for this); furthermore the first (video) packet triggers writing the actual header. Yet when the first video packet fails the checks performed on it, codec_ul (a value set based upon properties of the bitstream which necessitates actually inspecting packets) may be NULL; the next packet written (an audio packet) will then trigger outputting the header and this leads to a segfault because codec_ul is dereferenced (this is ticket #7993). Therefore this commit discards audio packets until a valid video packet has been received, fixing said ticket. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- This implements what I had originally in mind to fix said ticket and it is also what Baptiste Coudurier wanted [1]. The main obstacle in fixing it was fixing the mxf-d10-user-comments test (with this patch, the old test would output nothing at all). [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/257900.html libavformat/mxfenc.c | 7 +++++++ 1 file changed, 7 insertions(+)