diff mbox

[FFmpeg-devel,11/13] avformat/mux: Fix memleaks on errors II, improve documentation

Message ID 20190813024726.6596-11-andreas.rheinhardt@gmail.com
State Superseded
Headers show

Commit Message

Andreas Rheinhardt Aug. 13, 2019, 2:47 a.m. UTC
1. When an error happened in ff_interleave_add_packet during
interleaving a packet for output, said packet would not be unreferenced
in ff_interleave_add_packet, but would be zeroed in
av_interleaved_write_frame, which results in a memleak.
2. When no error happened in ff_interleave_add_packet during
interleaving a packet for output, the input packet will already be blank
(as if av_packet_unref had been applied to it), but it will
nevertheless be zeroed and initialized again. This is unnecessary.

Given the possibility of using other functions for interleavement than
ff_interleave_packet_per_dts, this commit sets and documents what
interleavement functions need to do: On success, the input packet (if
given) should be a blank packet on return. On failure, cleaning up will
be done by av_interleave_write_frame (where the already existing code
for error handling can be reused). ff_audio_rechunk_interleave has been
changed to abide by this. In order to keep these requirements
consistent, they are kept in one place that is referenced by the other
documentations.

Updating the documentation incidentally also removed another reference
to AVPacket's destructor.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/audiointerleave.c |  1 +
 libavformat/audiointerleave.h |  4 ++++
 libavformat/avformat.h        |  1 +
 libavformat/internal.h        | 16 +++++++---------
 libavformat/mux.c             | 30 +++++++++++-------------------
 5 files changed, 24 insertions(+), 28 deletions(-)
diff mbox

Patch

diff --git a/libavformat/audiointerleave.c b/libavformat/audiointerleave.c
index b7f4113b68..0c0dadf2f9 100644
--- a/libavformat/audiointerleave.c
+++ b/libavformat/audiointerleave.c
@@ -123,6 +123,7 @@  int ff_audio_rechunk_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt
                 aic->fifo_size = new_size;
             }
             av_fifo_generic_write(aic->fifo, pkt->data, pkt->size, NULL);
+            av_packet_unref(pkt);
         } else {
             // rewrite pts and dts to be decoded time line position
             pkt->pts = pkt->dts = aic->dts;
diff --git a/libavformat/audiointerleave.h b/libavformat/audiointerleave.h
index f28d5fefcc..99f427c05f 100644
--- a/libavformat/audiointerleave.h
+++ b/libavformat/audiointerleave.h
@@ -47,6 +47,10 @@  void ff_audio_interleave_close(AVFormatContext *s);
  *
  * @param get_packet function will output a packet when streams are correctly interleaved.
  * @param compare_ts function will compare AVPackets and decide interleaving order.
+ *
+ * Apart from these two functions, this function behaves like ff_interleave_packet_per_dts.
+ *
+ * @note  This function must not be used with uncoded audio frames.
  */
 int ff_audio_rechunk_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush,
                         int (*get_packet)(AVFormatContext *, AVPacket *, AVPacket *, int),
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 92c3a89f4b..12aadc0ce7 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -558,6 +558,7 @@  typedef struct AVOutputFormat {
     /**
      * A format-specific function for interleavement.
      * If unset, packets will be interleaved by dts.
+     * See the description of ff_interleave_packet_per_dts for details.
      */
     int (*interleave_packet)(struct AVFormatContext *, AVPacket *out,
                              AVPacket *in, int flush);
diff --git a/libavformat/internal.h b/libavformat/internal.h
index d6a039c497..ea6f65fecf 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -232,9 +232,9 @@  char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
 int ff_hex_to_data(uint8_t *data, const char *p);
 
 /**
- * Add packet to AVFormatContext->packet_buffer list, determining its
+ * Add packet to an AVFormatContext's packet_buffer list, determining its
  * interleaved position using compare() function argument.
- * @return 0, or < 0 on error
+ * @return 0, or < 0 on error. On success, pkt will be blank (as if unreferenced).
  */
 int ff_interleave_add_packet(AVFormatContext *s, AVPacket *pkt,
                              int (*compare)(AVFormatContext *, const AVPacket *, const AVPacket *));
@@ -497,15 +497,13 @@  int ff_framehash_write_header(AVFormatContext *s);
 int ff_read_packet(AVFormatContext *s, AVPacket *pkt);
 
 /**
- * Interleave a packet per dts in an output media file.
- *
- * Packets with pkt->destruct == av_destruct_packet will be freed inside this
- * function, so they cannot be used after it. Note that calling av_packet_unref()
- * on them is still safe.
+ * Interleave an AVPacket per dts so it can be muxed.
  *
- * @param s media file handle
+ * @param s   an AVFormatContext for output. in resp. out will be added to
+ *            resp. taken from its packet buffer.
  * @param out the interleaved packet will be output here
- * @param pkt the input packet
+ * @param in  the input packet. If not NULL will be a blank packet
+ *            (as if unreferenced) on success.
  * @param flush 1 if no further packets are available as input and all
  *              remaining packets should be output
  * @return 1 if a packet was output, 0 if no packet could be output,
diff --git a/libavformat/mux.c b/libavformat/mux.c
index c524c0886c..55fa0a184c 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1164,20 +1164,13 @@  int ff_interleaved_peek(AVFormatContext *s, int stream,
 
 /**
  * Interleave an AVPacket correctly so it can be muxed.
- * @param out the interleaved packet will be output here
- * @param in the input packet
- * @param flush 1 if no further packets are available as input and all
- *              remaining packets should be output
- * @return 1 if a packet was output, 0 if no packet could be output,
- *         < 0 if an error occurred
+ *
+ * See the description of ff_interleave_packet_per_dts for details.
  */
 static int interleave_packet(AVFormatContext *s, AVPacket *out, AVPacket *in, int flush)
 {
     if (s->oformat->interleave_packet) {
-        int ret = s->oformat->interleave_packet(s, out, in, flush);
-        if (in)
-            av_packet_unref(in);
-        return ret;
+        return s->oformat->interleave_packet(s, out, in, flush);
     } else
         return ff_interleave_packet_per_dts(s, out, in, flush);
 }
@@ -1220,14 +1213,13 @@  int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt)
 
     for (;; ) {
         AVPacket opkt;
-        int ret = interleave_packet(s, &opkt, pkt, flush);
-        if (pkt) {
-            memset(pkt, 0, sizeof(*pkt));
-            av_init_packet(pkt);
-            pkt = NULL;
-        }
-        if (ret <= 0) //FIXME cleanup needed for ret<0 ?
-            return ret;
+        ret = interleave_packet(s, &opkt, pkt, flush);
+        if (ret < 0)
+            goto fail;
+        if (!ret)
+            return 0;
+
+        pkt = NULL;
 
         ret = write_packet(s, &opkt);
         if (ret >= 0)
@@ -1244,7 +1236,7 @@  fail:
     // This is a deviation from the usual behaviour
     // of av_interleaved_write_frame: We leave cleaning
     // up uncoded frames to write_uncoded_frame_internal.
-    if (!(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME))
+    if (pkt && !(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME))
         av_packet_unref(pkt);
     return ret;
 }