[FFmpeg-devel] [PATCH 1/6] avformat/mux: Make uncoded frames av_packet_unref() compatible
James Almer
jamrial at gmail.com
Sun Apr 12 00:39:20 EEST 2020
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.
> 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".
>>
>
> _______________________________________________
> 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".
>
More information about the ffmpeg-devel
mailing list