diff mbox series

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

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
Related show

Checks

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

Commit Message

Andreas Rheinhardt Dec. 4, 2020, 11:48 p.m. UTC
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(+)

Comments

Marton Balint Dec. 5, 2020, 10:30 a.m. UTC | #1
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".
Andreas Rheinhardt Dec. 6, 2020, 3:48 a.m. UTC | #2
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
>>
Marton Balint Dec. 6, 2020, 3:04 p.m. UTC | #3
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 mbox series

Patch

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) {