| Message ID | 20200412122158.4846-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | ad1dc918a010d9d75e8babbe42a3996e1af00cb0 |
| Headers | show |
| Series | [FFmpeg-devel,v3] 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 |
Andreas Rheinhardt (12020-04-12): > 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 actually making uncoded frames refcounted. > In order not to rely on sizeof(AVFrame) (which is not part of the public > API and so must not be used here in libavformat) the packet's data is > changed to a (padded) buffer containing just a pointer to an AVFrame. > Said buffer is owned by an AVBuffer with a custom free function that > frees the frame as well as the buffer. Thereby the pointer/the AVBuffer > owns the AVFrame. > > Said ownership can actually be transferred by copying and resetting > the pointer, as might happen when actually writing the uncoded frames > in AVOutputFormat.write_uncoded_frame() (although currently no muxer > makes use of this possibility). > > This makes packets containing uncoded frames compatible with > av_packet_unref(). This already has three advantages 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. > > Suggested-by: Marton Balint <cus@passwd.hu> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > How embarrassing! The earlier version forgot to check the allocation. I am confused: does it not make unwrapped frames behave exactly the same as wrapped frames? AFAIU, Marton intends to remove all this code, and I think he is right, because it was a hack. Regards,
On Tue, 14 Apr 2020, Nicolas George wrote: > Andreas Rheinhardt (12020-04-12): >> 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 actually making uncoded frames refcounted. >> In order not to rely on sizeof(AVFrame) (which is not part of the public >> API and so must not be used here in libavformat) the packet's data is >> changed to a (padded) buffer containing just a pointer to an AVFrame. >> Said buffer is owned by an AVBuffer with a custom free function that >> frees the frame as well as the buffer. Thereby the pointer/the AVBuffer >> owns the AVFrame. >> >> Said ownership can actually be transferred by copying and resetting >> the pointer, as might happen when actually writing the uncoded frames >> in AVOutputFormat.write_uncoded_frame() (although currently no muxer >> makes use of this possibility). >> >> This makes packets containing uncoded frames compatible with >> av_packet_unref(). This already has three advantages 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. >> >> Suggested-by: Marton Balint <cus@passwd.hu> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> How embarrassing! The earlier version forgot to check the allocation. > > I am confused: does it not make unwrapped frames behave exactly the same > as wrapped frames? There is a slight difference, for WRAPPED_AVFRAME packets the AVFrame is stored in the data, for uncoded_frames packets only a pointer to AVFrame is stored in data. > > AFAIU, Marton intends to remove all this code, and I think he is right, > because it was a hack. Yes, but full removal can only happen at the next bump, so until that this should be fixed. Also this patchset has been delayed for a very long time now, I really like to see it applied as soon as possible, even if we further change uncoded_frames support later or around the time of the API bump. Thanks, Marton
Marton Balint (12020-04-16): > Yes, but full removal can only happen at the next bump, so until that this > should be fixed. Also this patchset has been delayed for a very long time > now, I really like to see it applied as soon as possible, even if we further > change uncoded_frames support later or around the time of the API bump. Ok, no objection from me. Regards,
Marton Balint: > > > On Tue, 14 Apr 2020, Nicolas George wrote: > >> Andreas Rheinhardt (12020-04-12): >>> 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 actually making uncoded frames refcounted. >>> In order not to rely on sizeof(AVFrame) (which is not part of the public >>> API and so must not be used here in libavformat) the packet's data is >>> changed to a (padded) buffer containing just a pointer to an AVFrame. >>> Said buffer is owned by an AVBuffer with a custom free function that >>> frees the frame as well as the buffer. Thereby the pointer/the AVBuffer >>> owns the AVFrame. >>> >>> Said ownership can actually be transferred by copying and resetting >>> the pointer, as might happen when actually writing the uncoded frames >>> in AVOutputFormat.write_uncoded_frame() (although currently no muxer >>> makes use of this possibility). >>> >>> This makes packets containing uncoded frames compatible with >>> av_packet_unref(). This already has three advantages 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. >>> >>> Suggested-by: Marton Balint <cus@passwd.hu> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> How embarrassing! The earlier version forgot to check the allocation. >> >> I am confused: does it not make unwrapped frames behave exactly the same >> as wrapped frames? > > There is a slight difference, for WRAPPED_AVFRAME packets the AVFrame is > stored in the data, for uncoded_frames packets only a pointer to AVFrame > is stored in data. > >> >> AFAIU, Marton intends to remove all this code, and I think he is right, >> because it was a hack. > > Yes, but full removal can only happen at the next bump, so until that > this should be fixed. Yes (and actually it can only be removed after a deprecation period which (if honoured) would postpone the removal by another major version bump). And the way it is done here means that the rest of the code is pretty much shielded from uncoded frames, so that you won't have to think "do I need to treat uncoded frames specially here?" in your patchset. > Also this patchset has been delayed for a very > long time now, I really like to see it applied as soon as possible, even > if we further change uncoded_frames support later or around the time of > the API bump. > So is anyone against me applying this? If not, I'll apply it tomorrow. - Andreas
diff --git a/libavformat/mux.c b/libavformat/mux.c index cc2d1e275a..bb54fd5528 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 @@ -650,7 +644,7 @@ static int compute_muxer_pkt_fields(AVFormatContext *s, AVStream *st, AVPacket * switch (st->codecpar->codec_type) { case AVMEDIA_TYPE_AUDIO: frame_size = (pkt->flags & AV_PKT_FLAG_UNCODED_FRAME) ? - ((AVFrame *)pkt->data)->nb_samples : + (*(AVFrame **)pkt->data)->nb_samples : av_get_audio_frame_duration(st->codec, pkt->size); /* HACK/FIXME, we skip the initial 0 size packets as they are most @@ -746,10 +740,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); - ret = s->oformat->write_uncoded_frame(s, pkt->stream_index, &frame, 0); - av_frame_free(&frame); + AVFrame **frame = (AVFrame **)pkt->data; + av_assert0(pkt->size == sizeof(*frame)); + ret = s->oformat->write_uncoded_frame(s, pkt->stream_index, frame, 0); + av_packet_unref(pkt); } else { ret = s->oformat->write_packet(s, pkt); } @@ -926,14 +920,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 +1308,45 @@ int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt, return ret; } +static void uncoded_frame_free(void *unused, uint8_t *data) +{ + av_frame_free((AVFrame **)data); + av_free(data); +} + 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 { + size_t bufsize = sizeof(frame) + AV_INPUT_BUFFER_PADDING_SIZE; + AVFrame **framep = av_mallocz(bufsize); + + if (!framep) + goto fail; pktp = &pkt; av_init_packet(&pkt); - pkt.data = (void *)frame; - pkt.size = UNCODED_FRAME_PACKET_SIZE; + pkt.buf = av_buffer_create((void *)framep, bufsize, + uncoded_frame_free, NULL, 0); + if (!pkt.buf) { + av_free(framep); + fail: + av_frame_free(&frame); + return AVERROR(ENOMEM); + } + *framep = frame; + + pkt.data = (void *)framep; + 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 actually making uncoded frames refcounted. In order not to rely on sizeof(AVFrame) (which is not part of the public API and so must not be used here in libavformat) the packet's data is changed to a (padded) buffer containing just a pointer to an AVFrame. Said buffer is owned by an AVBuffer with a custom free function that frees the frame as well as the buffer. Thereby the pointer/the AVBuffer owns the AVFrame. Said ownership can actually be transferred by copying and resetting the pointer, as might happen when actually writing the uncoded frames in AVOutputFormat.write_uncoded_frame() (although currently no muxer makes use of this possibility). This makes packets containing uncoded frames compatible with av_packet_unref(). This already has three advantages 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. Suggested-by: Marton Balint <cus@passwd.hu> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- How embarrassing! The earlier version forgot to check the allocation. libavformat/mux.c | 56 ++++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 22 deletions(-)