James Almer: > 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. > I know. And I actually thought that I don't make it worse because UNCODED_FRAME_PACKET_SIZE which is replaced here also uses sizeof(AVFrame). I did not want to set a negative size, because someone might add a check to av_buffer_create() that errors out in this case.
(Btw: libavcodec/wrapped_avframe.c also violates this.) >> 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". > _______________________________________________ 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".