On 4/11/2020 6:19 PM, Andreas Rheinhardt wrote: > Currently uncoded frames (i.e. packets whose data actually points to an > AVFrame) are not refcounted. As a consequence, calling av_packet_unref() > on them will not free them, but may simply make sure that they leak by > losing the pointer to the frame. > > This commit changes this by mimicking what is being done for wrapped > AVFrames: Ownership of the AVFrame is passed to a special AVBuffer with > a custom free function that properly frees the AVFrame. The packet is > equipped with an AVBufferRef referencing this AVBuffer. Thereby the > packet becomes compatible with av_packet_unref(). > > This already has three advantages, all in interleaved mode: > 1. If an error happens at the preparatory steps (before the packet is > put into the interleavement queue), the frame is properly freed. > 2. If the trailer is never written, the frames still in the > interleavement queue will now be properly freed by > ff_packet_list_free(). > 3. The custom code for moving the packet to the packet list in > ff_interleave_add_packet() can be removed. > > It will also simplify fixing further memleaks in future commits. > > Given that the AVFrame is now owned by an AVBuffer, the muxer may no > longer take ownership of the AVFrame, because the function used to call > the muxer when writing uncoded frames does not allow to transfer > ownership of the reference contained in the packet. (Changing the > function signature is not possible (except at a major version bump), > because most of these muxers reside in libavdevice.) But this is no loss > as none of the muxers ever made use of this. This change has been > documented. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com> > --- > The new semantic of AVOutputFormat.write_uncoded_frame() basically boils > down to treat frame as AVFrame * const *. I wonder whether I should > simply set it that way and remove the then redundant comment. > > libavformat/avformat.h | 4 ++-- > libavformat/mux.c | 43 ++++++++++++++++++++++++------------------ > 2 files changed, 27 insertions(+), 20 deletions(-) > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 39b99b4481..89207b9823 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -578,8 +578,8 @@ typedef struct AVOutputFormat { > * > * See av_write_uncoded_frame() for details. > * > - * The library will free *frame afterwards, but the muxer can prevent it > - * by setting the pointer to NULL. > + * The muxer must not change *frame, but it can keep the content of > **frame > + * by av_frame_move_ref(). > */ > int (*write_uncoded_frame)(struct AVFormatContext *, int stream_index, > AVFrame **frame, unsigned flags); > diff --git a/libavformat/mux.c b/libavformat/mux.c > index cc2d1e275a..712ba0c319 100644 > --- a/libavformat/mux.c > +++ b/libavformat/mux.c > @@ -550,12 +550,6 @@ fail: > > #define AV_PKT_FLAG_UNCODED_FRAME 0x2000 > > -/* Note: using sizeof(AVFrame) from outside lavu is unsafe in general, but > - it is only being used internally to this file as a consistency check. > - The value is chosen to be very unlikely to appear on its own and to cause > - immediate failure if used anywhere as a real size. */ > -#define UNCODED_FRAME_PACKET_SIZE (INT_MIN / 3 * 2 + (int)sizeof(AVFrame)) > - > > #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX > FF_DISABLE_DEPRECATION_WARNINGS > @@ -747,9 +741,10 @@ static int write_packet(AVFormatContext *s, AVPacket > *pkt) > > if ((pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) { > AVFrame *frame = (AVFrame *)pkt->data; > - av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE); > + av_assert0(pkt->size == sizeof(*frame));
sizeof(AVFrame) is not part of the ABI. This is not the first case of this violation happening in lavf, so it would be best to not make it worse. > ret = s->oformat->write_uncoded_frame(s, pkt->stream_index, &frame, > 0); > - av_frame_free(&frame); > + av_assert0((void*)frame == pkt->data); > + av_packet_unref(pkt); > } else { > ret = s->oformat->write_packet(s, pkt); > } > @@ -926,14 +921,9 @@ int ff_interleave_add_packet(AVFormatContext *s, > AVPacket *pkt, > this_pktl = av_malloc(sizeof(AVPacketList)); > if (!this_pktl) > return AVERROR(ENOMEM); > - if (pkt->flags & AV_PKT_FLAG_UNCODED_FRAME) { > - av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE); > - av_assert0(((AVFrame *)pkt->data)->buf); > - } else { > - if ((ret = av_packet_make_refcounted(pkt)) < 0) { > - av_free(this_pktl); > - return ret; > - } > + if ((ret = av_packet_make_refcounted(pkt)) < 0) { > + av_free(this_pktl); > + return ret; > } > > av_packet_move_ref(&this_pktl->pkt, pkt); > @@ -1319,22 +1309,39 @@ int ff_write_chained(AVFormatContext *dst, int > dst_stream, AVPacket *pkt, > return ret; > } > > +static void uncoded_frame_free(void *unused, uint8_t *data) > +{ > + AVFrame *frame = (AVFrame *)data; > + > + av_frame_free(&frame); > +} > + > static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, > AVFrame *frame, int interleaved) > { > AVPacket pkt, *pktp; > > av_assert0(s->oformat); > - if (!s->oformat->write_uncoded_frame) > + if (!s->oformat->write_uncoded_frame) { > + av_frame_free(&frame); > return AVERROR(ENOSYS); > + } > > if (!frame) { > pktp = NULL; > } else { > pktp = &pkt; > av_init_packet(&pkt); > + pkt.buf = av_buffer_create((uint8_t *)frame, sizeof(*frame), > + uncoded_frame_free, NULL, > + AV_BUFFER_FLAG_READONLY); > + if (!pkt.buf) { > + av_frame_free(&frame); > + return AVERROR(ENOMEM); > + } > + > pkt.data = (void *)frame; > - pkt.size = UNCODED_FRAME_PACKET_SIZE; > + pkt.size = sizeof(*frame); > pkt.pts = > pkt.dts = frame->pts; > pkt.duration = frame->pkt_duration; > _______________________________________________ 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".