[FFmpeg-devel] [PATCH 05/10] avformat/mux: Fix memleaks on errors II, improve documentation
Marton Balint
cus at passwd.hu
Wed Apr 1 00:41:21 EEST 2020
On Tue, 31 Mar 2020, Andreas Rheinhardt wrote:
> 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 at 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 --git a/libavformat/audiointerleave.c b/libavformat/audiointerleave.c
> index f10c83d7c9..d3b35d804e 100644
> --- a/libavformat/audiointerleave.c
> +++ b/libavformat/audiointerleave.c
> @@ -121,6 +121,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);
This is a little bit suspicous that you are changing the requirements of
the interleave_packet functions to always free its packets. This was not a
requirement until now, the semantics of the muxer callbacks can be
considered API. Yeah, probably has little impact, but still...
> } 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 0933310f4c..d21647b6b1 100644
> --- a/libavformat/audiointerleave.h
> +++ b/libavformat/audiointerleave.h
> @@ -48,6 +48,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 39b99b4481..8f7466931a 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -553,6 +553,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 332477a532..a861acdc2a 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -230,9 +230,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).
I'd lose the "as if"
> */
> int ff_interleave_add_packet(AVFormatContext *s, AVPacket *pkt,
> int (*compare)(AVFormatContext *, const AVPacket *, const AVPacket *));
> @@ -495,15 +495,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.
I don't really understand this. What is "resp." here?
> * @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.
I'd lose the "as if" here as well.
> * @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 a0ebcec119..79731d3008 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -1166,20 +1166,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);
If you want to keep this, and go ahead with the new expectations of the
interleave_packet callback, then let's do an av_assert0 that in is a blank
packet here on success.
> } else
> return ff_interleave_packet_per_dts(s, out, in, flush);
> }
> @@ -1222,14 +1215,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);
>
> @@ -1242,7 +1234,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;
> }
> --
> 2.20.1
Regards,
Marton
More information about the ffmpeg-devel
mailing list