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.rheinha...@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-requ...@ffmpeg.org with subject "unsubscribe".
