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

[Marton Balint]

On Sat, 11 Apr 2020, James Almer wrote:

> On 4/11/2020 6:35 PM, Andreas Rheinhardt wrote:
>> 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).
>
> Ugh, yes, you're right. Guess it makes no difference, then.

Can't you simply store a pointer to an AVFrame in the data?

>
>> 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.)

Maybe wrapped_avframe should also be changed eventually to only store a 
pointer to an AVFrame? That would at least fix the issue that realloc-ing 
the packet data corrupts the AVFrame because of extended_data referencing 
the AVFrame itself.

Regards,
Marton