| Message ID | 20200411211955.20843-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avformat/mux: Make uncoded frames av_packet_unref() compatible | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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@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. > 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; >
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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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@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@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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@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
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@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
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)); 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;
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@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(-)