On 4/11/2020 6:35 PM, Andreas Rheinhardt wrote: > 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).
Ugh, yes, you're right. Guess it makes no difference, then. > 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". > _______________________________________________ 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".