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