On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
Marton Balint:
On Tue, 31 Mar 2020, Andreas Rheinhardt wrote:
If writing an uncoded frame fails at the preparatory steps of
av_[interleaved_]write_frame(), the frame would be either not freed
at all in case of av_write_frame() or would leak when the fake packet
would be unreferenced like an ordinary packet.
There is also a memleak when the output format is not suited for
writing uncoded frames as the documentation states that ownership of the
frame passes to av_[interleaved_]write_uncoded_frame(). Both of these
memleaks have been fixed.
Yuck, does using uncoded_frames muxing have any benefit over using
AV_CODEC_ID_WRAPPED_AVFRAME with normal muxing? If not, then I'd very
much like to see uncoded frames dropped with the next bump...
I'd like to see them dropped, too, but the earliest possible time at
which they can be removed is the bump after the next major bump.
I tried to find a project on the internet which is using this API, failed
to find one. Therefore I doubt that it is reasonable to keep it after the
API bump (if we decide to drop it), it only makes sense to keep API
deprectated longer which is indeed used in the wild.
Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com>
---
libavformat/mux.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/libavformat/mux.c b/libavformat/mux.c
index 814d773b9d..a0ebcec119 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1239,7 +1239,11 @@ int av_interleaved_write_frame(AVFormatContext
*s, AVPacket *pkt)
return ret;
}
fail:
- av_packet_unref(pkt);
+ // This is a deviation from the usual behaviour
+ // of av_interleaved_write_frame: We leave cleaning
+ // up uncoded frames to write_uncoded_frame_internal.
+ if (!(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME))
+ av_packet_unref(pkt);
return ret;
}
@@ -1324,10 +1328,13 @@ static int
write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
AVFrame *frame, int interleaved)
{
AVPacket pkt, *pktp;
+ int ret;
av_assert0(s->oformat);
- if (!s->oformat->write_uncoded_frame)
- return AVERROR(ENOSYS);
+ if (!s->oformat->write_uncoded_frame) {
+ ret = AVERROR(ENOSYS);
+ goto free;
+ }
if (!frame) {
pktp = NULL;
@@ -1343,8 +1350,14 @@ static int
write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME;
}
- return interleaved ? av_interleaved_write_frame(s, pktp) :
- av_write_frame(s, pktp);
+ ret = interleaved ? av_interleaved_write_frame(s, pktp) :
+ av_write_frame(s, pktp);
+ if (ret < 0 && pktp) {
+ frame = (AVFrame*)pktp->data;
+ free:
I think we always align labels to the start of the line.
+ av_frame_free(&frame);
+ }
+ return ret;
}
int av_write_uncoded_frame(AVFormatContext *s, int stream_index,
LGTM, thanks.
It is actually not good at all, but I only found this out later: As the
commit message says, this fixes leaks if one of the preparatory steps
fails. But if there is an error when actually writing the frame, then
this will lead to a double free in the non-interleaved codepath is used
because of:
AVFrame *frame = (AVFrame *)pkt->data;
av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE);
ret = s->oformat->write_uncoded_frame(s, pkt->stream_index,
&frame, 0);
av_frame_free(&frame);
(Both times frame should be replaced by (AVFrame **)&pkt->data.)
And then the patch for no longer modifying packets we don't own needs to
be adapted, too: Using a spare packet for uncoded frames is not only
unnecessary, but harmful, because then the original packet would still
contain a dangling pointer to the frame even after it has been freed.
So far this stuff is easily fixable (and I have fixed it locally), but
there is more:
1. As I already said, there is a leak in case the functions for
interleaved output are used if the trailer is never written (e.g.
because an error occurs during an earlier write operation), because
ff_packet_list_free() is not aware of uncoded frames.
2. This is all very fragile. E.g. if one of these warnings in
write_packet() would be transformed into errors, then (all other things
equal) uncoded frames would leak if they have been written via the
interleaved codepath.
3. I do not even know whether the approach taken here (namely leaving
cleaning up to the callers in order to minimize the places where one
does cleanup) is compatible with Marton's patchset.
I see two ways going forward:
a) The way uncoded frames are handled is changed so that it respects the
ordinary AVBuffer-API: Ownership of the AVFrame is passed to an AVBuffer
with a custom free function (like it is for wrapped AVFrames). This will
automatically solve the first problem and it will simplify freeing more
generally (no constant checks for whether this is an uncoded frame).
The downsides of this are that it involves two allocations* per frame
and that the muxer must no longer have the ability to keep the AVFrame,
because the AVFrame is owned by an AVBuffer and the signature of
write_uncoded_frame does not allow to pass this reference to the
muxer.** But none of these muxers make use of this at all (and
av_frame_move_ref() is still fine).
b) Marton's patches get applied and I figure out later what the most
efficient way to fix these memleaks is. Fixing 1. would then imply that
the uncoded frame-hack can no longer be contained in mux.c.
The third option is to keep it broken, and apply the rest of the patch
series. Yeah, a) seems like the best way, if you have the capacity to do
it, but I'd simply ignore that it is broken, because IMHO it is simply a
bad design which does not worth my time.
My preferred approach is a).
- Andreas
*: In 436f00b10c062b75c7aab276c4a7d64524bd0444 Marton modified the
wrapped AVFrame encoder to create a new packet with padding. I don't
know if this would be needed here. I don't see any function (except the
muxer, of course) which actually looks at the packet's data in the
codepath here. Marton, do you still remember the exact function that
needed the padding?
Padding is required because we don't want av_buffer_realloc() in
avcodec_encode_video/audio to realloc the packet buf, because it has
internal pointers (extended_data) which points to the AVFrame itself, so
a simple memcpy on the data to another place breaks it.
**: Said signature is not part of the public API, but the muxers that
support writing uncoded frames reside in libavdevice and therefore we
can't simply change it.
I'd say keep signature, but change documentation. And also assert() if
AVFrame is reset to NULL. Technically a break, but IMHO acceptable.
Regards,
Marton
_______________________________________________
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".
