[FFmpeg-devel] [PATCH 5/6] avformat/mux: Don't modify packets we don't own

[Andreas Rheinhardt]

The documentation of av_write_frame() explicitly states that the function
doesn't take ownership of the packets sent to it; while av_write_frame()
does not directly unreference the packets after having written them, it
nevertheless modifies the packet in various ways:
1. The timestamps might be modified either by prepare_input_packet() or
compute_muxer_pkt_fields().
2. If a bitstream filter gets applied, it takes ownership of the
reference and the side-data in the packet sent to it.
In case of do_packet_auto_bsf(), the end result is that the returned packet
contains the output of the last bsf in the chain. If an error happens,
a blank packet will be returned; a packet may also simply not lead to
any output (vp9_superframe).
This also implies that side data needs to be really copied and can't be
shared with the input packet.
The method choosen here minimizes copying of data: When the input isn't
refcounted and no bitstream filter is applied, the packet's data will
not be copied.

Notice that packets that contain uncoded frames are exempt from this
because these packets are not owned by and returned to the user. This
also moves unreferencing the packets containing uncoded frames to
av_write_frame() in the noninterleaved codepath; in the interleaved
codepath, these packets are already freed in av_interleaved_write_frame(),
so that unreferencing the packets in write_uncoded_frame_internal() is
no longer needed. It has been removed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
---
I was actually surprised when I realized how freeing uncoded frames in
the noninterleaved codepath could be left to av_write_frame().

 libavformat/mux.c | 48 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/libavformat/mux.c b/libavformat/mux.c
index cae9f42d11..48c0d4cd5f 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -874,11 +874,12 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) {
     return 1;
 }
 
-int av_write_frame(AVFormatContext *s, AVPacket *pkt)
+int av_write_frame(AVFormatContext *s, AVPacket *in)
 {
+    AVPacket local_pkt, *pkt = &local_pkt;
     int ret;
 
-    if (!pkt) {
+    if (!in) {
         if (s->oformat->flags & AVFMT_ALLOW_FLUSH) {
             ret = s->oformat->write_packet(s, NULL);
             flush_if_needed(s);
@@ -889,22 +890,49 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt)
         return 1;
     }
 
+    if (in->flags & AV_PKT_FLAG_UNCODED_FRAME) {
+        pkt = in;
+    } else {
+        /* We don't own in, so we have to make sure not to modify it.
+         * The following avoids copying in's data unnecessarily.
+         * Copying side data is unavoidable as a bitstream filter
+         * may change it, e.g. free it on errors. */
+        pkt->buf  = NULL;
+        pkt->data = in->data;
+        pkt->size = in->size;
+        ret = av_packet_copy_props(pkt, in);
+        if (ret < 0)
+            return ret;
+        if (in->buf) {
+            pkt->buf = av_buffer_ref(in->buf);
+            if (!pkt->buf) {
+                ret = AVERROR(ENOMEM);
+                goto fail;
+            }
+        }
+    }
+
     ret = prepare_input_packet(s, pkt);
     if (ret < 0)
-        return ret;
+        goto fail;
 
     ret = do_packet_auto_bsf(s, pkt);
     if (ret <= 0)
-        return ret;
+        goto fail;
 
 #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX
     ret = compute_muxer_pkt_fields(s, s->streams[pkt->stream_index], pkt);
 
     if (ret < 0 && !(s->oformat->flags & AVFMT_NOTIMESTAMPS))
-        return ret;
+        goto fail;
 #endif
 
-    return write_packet(s, pkt);
+    ret = write_packet(s, pkt);
+
+fail:
+    // Uncoded frames using the noninterleaved codepath are freed here
+    av_packet_unref(pkt);
+    return ret;
 }
 
 #define CHUNK_START 0x1000
@@ -1319,7 +1347,6 @@ 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) {
@@ -1349,11 +1376,8 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
         pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME;
     }
 
-    ret = interleaved ? av_interleaved_write_frame(s, pktp) :
-                        av_write_frame(s, pktp);
-    if (pktp)
-        av_packet_unref(pktp);
-    return ret;
+    return interleaved ? av_interleaved_write_frame(s, pktp) :
+                         av_write_frame(s, pktp);
 }
 
 int av_write_uncoded_frame(AVFormatContext *s, int stream_index,
-- 
2.20.1