[FFmpeg-devel] [PATCH 04/10] avformat/mux: Fix leaks of uncoded frames on errors

[Marton Balint]

On Tue, 31 Mar 2020, Andreas Rheinhardt wrote:

> If writing an uncoded frame fails at the preparatory steps of
> av_[interleaved_]write_frame(), the frame would be either not freed
> at all in case of av_write_frame() or would leak when the fake packet
> would be unreferenced like an ordinary packet.
> There is also a memleak when the output format is not suited for
> writing uncoded frames as the documentation states that ownership of the
> frame passes to av_[interleaved_]write_uncoded_frame(). Both of these
> memleaks have been fixed.

Yuck, does using uncoded_frames muxing have any benefit over using 
AV_CODEC_ID_WRAPPED_AVFRAME with normal muxing? If not, then I'd very much 
like to see uncoded frames dropped with the next bump...

>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> libavformat/mux.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index 814d773b9d..a0ebcec119 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -1239,7 +1239,11 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt)
>             return ret;
>     }
> fail:
> -    av_packet_unref(pkt);
> +    // 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))
> +        av_packet_unref(pkt);
>     return ret;
> }
> 
> @@ -1324,10 +1328,13 @@ 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)
> -        return AVERROR(ENOSYS);
> +    if (!s->oformat->write_uncoded_frame) {
> +        ret = AVERROR(ENOSYS);
> +        goto free;
> +    }
>
>     if (!frame) {
>         pktp = NULL;
> @@ -1343,8 +1350,14 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>         pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME;
>     }
> 
> -    return interleaved ? av_interleaved_write_frame(s, pktp) :
> -                         av_write_frame(s, pktp);
> +    ret = interleaved ? av_interleaved_write_frame(s, pktp) :
> +                        av_write_frame(s, pktp);
> +    if (ret < 0 && pktp) {
> +        frame = (AVFrame*)pktp->data;
> +    free:

I think we always align labels to the start of the line.

> +        av_frame_free(&frame);
> +    }
> +    return ret;
> }
> 
> int av_write_uncoded_frame(AVFormatContext *s, int stream_index,

LGTM, thanks.

Marton