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

[Andreas Rheinhardt]

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?
> 
That's a very good idea. Should work if I am not mistaken. (The
situation here is actually pretty simple: The refcount of the AVBuffer
owning the AVFrame/the pointer to an AVFrame will always be 1; but it
should work more generally.)

- Andreas

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