[FFmpeg-devel] [PATCH 1/6] avformat/mux: Make uncoded frames av_packet_unref() compatible

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Sun Apr 12 00:19:50 EEST 2020


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.rheinhardt at 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));
         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;
-- 
2.20.1



More information about the ffmpeg-devel mailing list