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

[Andreas Rheinhardt]

The documentation of av_write_frame explicitly states that the function
does not 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 av_bsf_send_packet.
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,
an empty 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.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
---
I am not sure about this one. If one understands ownership of a packet
as "the right to modify a packet as well as the obligation to ultimately
free the data" (in case the data is shared, one of course needs to make
it writable before modifying it), then it is obvious that av_write_frame
needs to preserve the original packet given to it; that was my
understanding when writing this patch. But Hendrik has said that
"ownership" actually only comes with the obligation to free the data and
that av_write_frame does not taking ownership of the packet does not
imply that the caller may expect the packet to be returned unmodified.
If the latter is correct, then this patch is moot, of course.

 libavformat/mux.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/libavformat/mux.c b/libavformat/mux.c
index 55fa0a184c..2dc7fc2dcb 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -872,11 +872,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);
@@ -887,19 +888,36 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt)
         return 1;
     }
 
+    /* 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->data = in->data;
+    pkt->size = in->size;
+    if (in->buf) {
+        pkt->buf = av_buffer_ref(in->buf);
+        if (!pkt->buf)
+            return AVERROR(ENOMEM);
+    } else {
+        pkt->buf = NULL;
+    }
+    ret = av_packet_copy_props(pkt, in);
+    if (ret < 0)
+        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
 
     ret = write_packet(s, pkt);
@@ -908,6 +926,9 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt)
 
     if (ret >= 0)
         s->streams[pkt->stream_index]->nb_frames++;
+
+fail:
+    av_packet_unref(pkt);
     return ret;
 }
 
-- 
2.21.0