[FFmpeg-devel] [PATCH 1/6] avformat/mux: Make uncoded frames av_packet_unref() compatible

[James Almer]

On 4/11/2020 6:19 PM, Andreas Rheinhardt wrote:
> Currently uncoded frames (i.e. packets whose data actually points to an
> AVFrame) are not refcounted. As a consequence, calling av_packet_unref()
> on them will not free them, but may simply make sure that they leak by
> losing the pointer to the frame.
> 
> This commit changes this by mimicking what is being done for wrapped
> AVFrames: Ownership of the AVFrame is passed to a special AVBuffer with
> a custom free function that properly frees the AVFrame. The packet is
> equipped with an AVBufferRef referencing this AVBuffer. Thereby the
> packet becomes compatible with av_packet_unref().
> 
> This already has three advantages, all in interleaved mode:
> 1. If an error happens at the preparatory steps (before the packet is
> put into the interleavement queue), the frame is properly freed.
> 2. If the trailer is never written, the frames still in the
> interleavement queue will now be properly freed by
> ff_packet_list_free().
> 3. The custom code for moving the packet to the packet list in
> ff_interleave_add_packet() can be removed.
> 
> It will also simplify fixing further memleaks in future commits.
> 
> Given that the AVFrame is now owned by an AVBuffer, the muxer may no
> longer take ownership of the AVFrame, because the function used to call
> the muxer when writing uncoded frames does not allow to transfer
> ownership of the reference contained in the packet. (Changing the
> function signature is not possible (except at a major version bump),
> because most of these muxers reside in libavdevice.) But this is no loss
> as none of the muxers ever made use of this. This change has been
> documented.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> The new semantic of AVOutputFormat.write_uncoded_frame() basically boils
> down to treat frame as AVFrame * const *. I wonder whether I should
> simply set it that way and remove the then redundant comment.
> 
>  libavformat/avformat.h |  4 ++--
>  libavformat/mux.c      | 43 ++++++++++++++++++++++++------------------
>  2 files changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 39b99b4481..89207b9823 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -578,8 +578,8 @@ typedef struct AVOutputFormat {
>       *
>       * See av_write_uncoded_frame() for details.
>       *
> -     * The library will free *frame afterwards, but the muxer can prevent it
> -     * by setting the pointer to NULL.
> +     * The muxer must not change *frame, but it can keep the content of **frame
> +     * by av_frame_move_ref().
>       */
>      int (*write_uncoded_frame)(struct AVFormatContext *, int stream_index,
>                                 AVFrame **frame, unsigned flags);
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index cc2d1e275a..712ba0c319 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -550,12 +550,6 @@ fail:
>  
>  #define AV_PKT_FLAG_UNCODED_FRAME 0x2000
>  
> -/* Note: using sizeof(AVFrame) from outside lavu is unsafe in general, but
> -   it is only being used internally to this file as a consistency check.
> -   The value is chosen to be very unlikely to appear on its own and to cause
> -   immediate failure if used anywhere as a real size. */
> -#define UNCODED_FRAME_PACKET_SIZE (INT_MIN / 3 * 2 + (int)sizeof(AVFrame))
> -
>  
>  #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX
>  FF_DISABLE_DEPRECATION_WARNINGS
> @@ -747,9 +741,10 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt)
>  
>      if ((pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) {
>          AVFrame *frame = (AVFrame *)pkt->data;
> -        av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE);
> +        av_assert0(pkt->size == sizeof(*frame));

sizeof(AVFrame) is not part of the ABI.

This is not the first case of this violation happening in lavf, so it
would be best to not make it worse.

>          ret = s->oformat->write_uncoded_frame(s, pkt->stream_index, &frame, 0);
> -        av_frame_free(&frame);
> +        av_assert0((void*)frame == pkt->data);
> +        av_packet_unref(pkt);
>      } else {
>          ret = s->oformat->write_packet(s, pkt);
>      }
> @@ -926,14 +921,9 @@ int ff_interleave_add_packet(AVFormatContext *s, AVPacket *pkt,
>      this_pktl    = av_malloc(sizeof(AVPacketList));
>      if (!this_pktl)
>          return AVERROR(ENOMEM);
> -    if (pkt->flags & AV_PKT_FLAG_UNCODED_FRAME) {
> -        av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE);
> -        av_assert0(((AVFrame *)pkt->data)->buf);
> -    } else {
> -        if ((ret = av_packet_make_refcounted(pkt)) < 0) {
> -            av_free(this_pktl);
> -            return ret;
> -        }
> +    if ((ret = av_packet_make_refcounted(pkt)) < 0) {
> +        av_free(this_pktl);
> +        return ret;
>      }
>  
>      av_packet_move_ref(&this_pktl->pkt, pkt);
> @@ -1319,22 +1309,39 @@ int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt,
>      return ret;
>  }
>  
> +static void uncoded_frame_free(void *unused, uint8_t *data)
> +{
> +    AVFrame *frame = (AVFrame *)data;
> +
> +    av_frame_free(&frame);
> +}
> +
>  static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>                                          AVFrame *frame, int interleaved)
>  {
>      AVPacket pkt, *pktp;
>  
>      av_assert0(s->oformat);
> -    if (!s->oformat->write_uncoded_frame)
> +    if (!s->oformat->write_uncoded_frame) {
> +        av_frame_free(&frame);
>          return AVERROR(ENOSYS);
> +    }
>  
>      if (!frame) {
>          pktp = NULL;
>      } else {
>          pktp = &pkt;
>          av_init_packet(&pkt);
> +        pkt.buf = av_buffer_create((uint8_t *)frame, sizeof(*frame),
> +                                   uncoded_frame_free, NULL,
> +                                   AV_BUFFER_FLAG_READONLY);
> +        if (!pkt.buf) {
> +            av_frame_free(&frame);
> +            return AVERROR(ENOMEM);
> +        }
> +
>          pkt.data = (void *)frame;
> -        pkt.size         = UNCODED_FRAME_PACKET_SIZE;
> +        pkt.size         = sizeof(*frame);
>          pkt.pts          =
>          pkt.dts          = frame->pts;
>          pkt.duration     = frame->pkt_duration;
>