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

[Andreas Rheinhardt]

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.
> 
I know. And I actually thought that I don't make it worse because
UNCODED_FRAME_PACKET_SIZE which is replaced here also uses
sizeof(AVFrame). I did not want to set a negative size, because someone
might add a check to av_buffer_create() that errors out in this case.

(Btw: libavcodec/wrapped_avframe.c also violates this.)

>>          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;
>>
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>