| Message ID | 20200331123745.6461-5-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libavformat/mux patches | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On Tue, 31 Mar 2020, Andreas Rheinhardt wrote: > If writing an uncoded frame fails at the preparatory steps of > av_[interleaved_]write_frame(), the frame would be either not freed > at all in case of av_write_frame() or would leak when the fake packet > would be unreferenced like an ordinary packet. > There is also a memleak when the output format is not suited for > writing uncoded frames as the documentation states that ownership of the > frame passes to av_[interleaved_]write_uncoded_frame(). Both of these > memleaks have been fixed. Yuck, does using uncoded_frames muxing have any benefit over using AV_CODEC_ID_WRAPPED_AVFRAME with normal muxing? If not, then I'd very much like to see uncoded frames dropped with the next bump... > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/mux.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/libavformat/mux.c b/libavformat/mux.c > index 814d773b9d..a0ebcec119 100644 > --- a/libavformat/mux.c > +++ b/libavformat/mux.c > @@ -1239,7 +1239,11 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt) > return ret; > } > fail: > - av_packet_unref(pkt); > + // This is a deviation from the usual behaviour > + // of av_interleaved_write_frame: We leave cleaning > + // up uncoded frames to write_uncoded_frame_internal. > + if (!(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) > + av_packet_unref(pkt); > return ret; > } > > @@ -1324,10 +1328,13 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, > AVFrame *frame, int interleaved) > { > AVPacket pkt, *pktp; > + int ret; > > av_assert0(s->oformat); > - if (!s->oformat->write_uncoded_frame) > - return AVERROR(ENOSYS); > + if (!s->oformat->write_uncoded_frame) { > + ret = AVERROR(ENOSYS); > + goto free; > + } > > if (!frame) { > pktp = NULL; > @@ -1343,8 +1350,14 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, > pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME; > } > > - return interleaved ? av_interleaved_write_frame(s, pktp) : > - av_write_frame(s, pktp); > + ret = interleaved ? av_interleaved_write_frame(s, pktp) : > + av_write_frame(s, pktp); > + if (ret < 0 && pktp) { > + frame = (AVFrame*)pktp->data; > + free: I think we always align labels to the start of the line. > + av_frame_free(&frame); > + } > + return ret; > } > > int av_write_uncoded_frame(AVFormatContext *s, int stream_index, LGTM, thanks. Marton
Marton Balint: > > > On Tue, 31 Mar 2020, Andreas Rheinhardt wrote: > >> If writing an uncoded frame fails at the preparatory steps of >> av_[interleaved_]write_frame(), the frame would be either not freed >> at all in case of av_write_frame() or would leak when the fake packet >> would be unreferenced like an ordinary packet. >> There is also a memleak when the output format is not suited for >> writing uncoded frames as the documentation states that ownership of the >> frame passes to av_[interleaved_]write_uncoded_frame(). Both of these >> memleaks have been fixed. > > Yuck, does using uncoded_frames muxing have any benefit over using > AV_CODEC_ID_WRAPPED_AVFRAME with normal muxing? If not, then I'd very > much like to see uncoded frames dropped with the next bump... > I'd like to see them dropped, too, but the earliest possible time at which they can be removed is the bump after the next major bump. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/mux.c | 23 ++++++++++++++++++----- >> 1 file changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/libavformat/mux.c b/libavformat/mux.c >> index 814d773b9d..a0ebcec119 100644 >> --- a/libavformat/mux.c >> +++ b/libavformat/mux.c >> @@ -1239,7 +1239,11 @@ int av_interleaved_write_frame(AVFormatContext >> *s, AVPacket *pkt) >> return ret; >> } >> fail: >> - av_packet_unref(pkt); >> + // This is a deviation from the usual behaviour >> + // of av_interleaved_write_frame: We leave cleaning >> + // up uncoded frames to write_uncoded_frame_internal. >> + if (!(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) >> + av_packet_unref(pkt); >> return ret; >> } >> >> @@ -1324,10 +1328,13 @@ static int >> write_uncoded_frame_internal(AVFormatContext *s, int stream_index, >> AVFrame *frame, int interleaved) >> { >> AVPacket pkt, *pktp; >> + int ret; >> >> av_assert0(s->oformat); >> - if (!s->oformat->write_uncoded_frame) >> - return AVERROR(ENOSYS); >> + if (!s->oformat->write_uncoded_frame) { >> + ret = AVERROR(ENOSYS); >> + goto free; >> + } >> >> if (!frame) { >> pktp = NULL; >> @@ -1343,8 +1350,14 @@ static int >> write_uncoded_frame_internal(AVFormatContext *s, int stream_index, >> pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME; >> } >> >> - return interleaved ? av_interleaved_write_frame(s, pktp) : >> - av_write_frame(s, pktp); >> + ret = interleaved ? av_interleaved_write_frame(s, pktp) : >> + av_write_frame(s, pktp); >> + if (ret < 0 && pktp) { >> + frame = (AVFrame*)pktp->data; >> + free: > > I think we always align labels to the start of the line. > >> + av_frame_free(&frame); >> + } >> + return ret; >> } >> >> int av_write_uncoded_frame(AVFormatContext *s, int stream_index, > > LGTM, thanks. It is actually not good at all, but I only found this out later: As the commit message says, this fixes leaks if one of the preparatory steps fails. But if there is an error when actually writing the frame, then this will lead to a double free in the non-interleaved codepath is used because of: 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); (Both times frame should be replaced by (AVFrame **)&pkt->data.) And then the patch for no longer modifying packets we don't own needs to be adapted, too: Using a spare packet for uncoded frames is not only unnecessary, but harmful, because then the original packet would still contain a dangling pointer to the frame even after it has been freed. So far this stuff is easily fixable (and I have fixed it locally), but there is more: 1. As I already said, there is a leak in case the functions for interleaved output are used if the trailer is never written (e.g. because an error occurs during an earlier write operation), because ff_packet_list_free() is not aware of uncoded frames. 2. This is all very fragile. E.g. if one of these warnings in write_packet() would be transformed into errors, then (all other things equal) uncoded frames would leak if they have been written via the interleaved codepath. 3. I do not even know whether the approach taken here (namely leaving cleaning up to the callers in order to minimize the places where one does cleanup) is compatible with Marton's patchset. I see two ways going forward: a) The way uncoded frames are handled is changed so that it respects the ordinary AVBuffer-API: Ownership of the AVFrame is passed to an AVBuffer with a custom free function (like it is for wrapped AVFrames). This will automatically solve the first problem and it will simplify freeing more generally (no constant checks for whether this is an uncoded frame). The downsides of this are that it involves two allocations* per frame and that the muxer must no longer have the ability to keep the AVFrame, because the AVFrame is owned by an AVBuffer and the signature of write_uncoded_frame does not allow to pass this reference to the muxer.** But none of these muxers make use of this at all (and av_frame_move_ref() is still fine). b) Marton's patches get applied and I figure out later what the most efficient way to fix these memleaks is. Fixing 1. would then imply that the uncoded frame-hack can no longer be contained in mux.c. My preferred approach is a). - Andreas *: In 436f00b10c062b75c7aab276c4a7d64524bd0444 Marton modified the wrapped AVFrame encoder to create a new packet with padding. I don't know if this would be needed here. I don't see any function (except the muxer, of course) which actually looks at the packet's data in the codepath here. Marton, do you still remember the exact function that needed the padding? **: Said signature is not part of the public API, but the muxers that support writing uncoded frames reside in libavdevice and therefore we can't simply change it.
On Fri, 10 Apr 2020, Andreas Rheinhardt wrote: > Marton Balint: >> >> >> On Tue, 31 Mar 2020, Andreas Rheinhardt wrote: >> >>> If writing an uncoded frame fails at the preparatory steps of >>> av_[interleaved_]write_frame(), the frame would be either not freed >>> at all in case of av_write_frame() or would leak when the fake packet >>> would be unreferenced like an ordinary packet. >>> There is also a memleak when the output format is not suited for >>> writing uncoded frames as the documentation states that ownership of the >>> frame passes to av_[interleaved_]write_uncoded_frame(). Both of these >>> memleaks have been fixed. >> >> Yuck, does using uncoded_frames muxing have any benefit over using >> AV_CODEC_ID_WRAPPED_AVFRAME with normal muxing? If not, then I'd very >> much like to see uncoded frames dropped with the next bump... >> > I'd like to see them dropped, too, but the earliest possible time at > which they can be removed is the bump after the next major bump. I tried to find a project on the internet which is using this API, failed to find one. Therefore I doubt that it is reasonable to keep it after the API bump (if we decide to drop it), it only makes sense to keep API deprectated longer which is indeed used in the wild. > >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavformat/mux.c | 23 ++++++++++++++++++----- >>> 1 file changed, 18 insertions(+), 5 deletions(-) >>> >>> diff --git a/libavformat/mux.c b/libavformat/mux.c >>> index 814d773b9d..a0ebcec119 100644 >>> --- a/libavformat/mux.c >>> +++ b/libavformat/mux.c >>> @@ -1239,7 +1239,11 @@ int av_interleaved_write_frame(AVFormatContext >>> *s, AVPacket *pkt) >>> return ret; >>> } >>> fail: >>> - av_packet_unref(pkt); >>> + // This is a deviation from the usual behaviour >>> + // of av_interleaved_write_frame: We leave cleaning >>> + // up uncoded frames to write_uncoded_frame_internal. >>> + if (!(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) >>> + av_packet_unref(pkt); >>> return ret; >>> } >>> >>> @@ -1324,10 +1328,13 @@ static int >>> write_uncoded_frame_internal(AVFormatContext *s, int stream_index, >>> AVFrame *frame, int interleaved) >>> { >>> AVPacket pkt, *pktp; >>> + int ret; >>> >>> av_assert0(s->oformat); >>> - if (!s->oformat->write_uncoded_frame) >>> - return AVERROR(ENOSYS); >>> + if (!s->oformat->write_uncoded_frame) { >>> + ret = AVERROR(ENOSYS); >>> + goto free; >>> + } >>> >>> if (!frame) { >>> pktp = NULL; >>> @@ -1343,8 +1350,14 @@ static int >>> write_uncoded_frame_internal(AVFormatContext *s, int stream_index, >>> pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME; >>> } >>> >>> - return interleaved ? av_interleaved_write_frame(s, pktp) : >>> - av_write_frame(s, pktp); >>> + ret = interleaved ? av_interleaved_write_frame(s, pktp) : >>> + av_write_frame(s, pktp); >>> + if (ret < 0 && pktp) { >>> + frame = (AVFrame*)pktp->data; >>> + free: >> >> I think we always align labels to the start of the line. >> >>> + av_frame_free(&frame); >>> + } >>> + return ret; >>> } >>> >>> int av_write_uncoded_frame(AVFormatContext *s, int stream_index, >> >> LGTM, thanks. > > It is actually not good at all, but I only found this out later: As the > commit message says, this fixes leaks if one of the preparatory steps > fails. But if there is an error when actually writing the frame, then > this will lead to a double free in the non-interleaved codepath is used > because of: > 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); > > (Both times frame should be replaced by (AVFrame **)&pkt->data.) > > And then the patch for no longer modifying packets we don't own needs to > be adapted, too: Using a spare packet for uncoded frames is not only > unnecessary, but harmful, because then the original packet would still > contain a dangling pointer to the frame even after it has been freed. > > So far this stuff is easily fixable (and I have fixed it locally), but > there is more: > 1. As I already said, there is a leak in case the functions for > interleaved output are used if the trailer is never written (e.g. > because an error occurs during an earlier write operation), because > ff_packet_list_free() is not aware of uncoded frames. > 2. This is all very fragile. E.g. if one of these warnings in > write_packet() would be transformed into errors, then (all other things > equal) uncoded frames would leak if they have been written via the > interleaved codepath. > 3. I do not even know whether the approach taken here (namely leaving > cleaning up to the callers in order to minimize the places where one > does cleanup) is compatible with Marton's patchset. > > I see two ways going forward: > a) The way uncoded frames are handled is changed so that it respects the > ordinary AVBuffer-API: Ownership of the AVFrame is passed to an AVBuffer > with a custom free function (like it is for wrapped AVFrames). This will > automatically solve the first problem and it will simplify freeing more > generally (no constant checks for whether this is an uncoded frame). > The downsides of this are that it involves two allocations* per frame > and that the muxer must no longer have the ability to keep the AVFrame, > because the AVFrame is owned by an AVBuffer and the signature of > write_uncoded_frame does not allow to pass this reference to the > muxer.** But none of these muxers make use of this at all (and > av_frame_move_ref() is still fine). > b) Marton's patches get applied and I figure out later what the most > efficient way to fix these memleaks is. Fixing 1. would then imply that > the uncoded frame-hack can no longer be contained in mux.c. The third option is to keep it broken, and apply the rest of the patch series. Yeah, a) seems like the best way, if you have the capacity to do it, but I'd simply ignore that it is broken, because IMHO it is simply a bad design which does not worth my time. > > My preferred approach is a). > > - Andreas > > *: In 436f00b10c062b75c7aab276c4a7d64524bd0444 Marton modified the > wrapped AVFrame encoder to create a new packet with padding. I don't > know if this would be needed here. I don't see any function (except the > muxer, of course) which actually looks at the packet's data in the > codepath here. Marton, do you still remember the exact function that > needed the padding? Padding is required because we don't want av_buffer_realloc() in avcodec_encode_video/audio to realloc the packet buf, because it has internal pointers (extended_data) which points to the AVFrame itself, so a simple memcpy on the data to another place breaks it. > **: Said signature is not part of the public API, but the muxers that > support writing uncoded frames reside in libavdevice and therefore we > can't simply change it. I'd say keep signature, but change documentation. And also assert() if AVFrame is reset to NULL. Technically a break, but IMHO acceptable. Regards, Marton
diff --git a/libavformat/mux.c b/libavformat/mux.c index 814d773b9d..a0ebcec119 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -1239,7 +1239,11 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt) return ret; } fail: - av_packet_unref(pkt); + // This is a deviation from the usual behaviour + // of av_interleaved_write_frame: We leave cleaning + // up uncoded frames to write_uncoded_frame_internal. + if (!(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) + av_packet_unref(pkt); return ret; } @@ -1324,10 +1328,13 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, AVFrame *frame, int interleaved) { AVPacket pkt, *pktp; + int ret; av_assert0(s->oformat); - if (!s->oformat->write_uncoded_frame) - return AVERROR(ENOSYS); + if (!s->oformat->write_uncoded_frame) { + ret = AVERROR(ENOSYS); + goto free; + } if (!frame) { pktp = NULL; @@ -1343,8 +1350,14 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME; } - return interleaved ? av_interleaved_write_frame(s, pktp) : - av_write_frame(s, pktp); + ret = interleaved ? av_interleaved_write_frame(s, pktp) : + av_write_frame(s, pktp); + if (ret < 0 && pktp) { + frame = (AVFrame*)pktp->data; + free: + av_frame_free(&frame); + } + return ret; } int av_write_uncoded_frame(AVFormatContext *s, int stream_index,
If writing an uncoded frame fails at the preparatory steps of av_[interleaved_]write_frame(), the frame would be either not freed at all in case of av_write_frame() or would leak when the fake packet would be unreferenced like an ordinary packet. There is also a memleak when the output format is not suited for writing uncoded frames as the documentation states that ownership of the frame passes to av_[interleaved_]write_uncoded_frame(). Both of these memleaks have been fixed. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/mux.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)