[FFmpeg-devel] [PATCH] avformat/mxfenc: Discard audio until valid video has been received

[Marton Balint]

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