| Message ID | 20200411211955.20843-5-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| 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 Sat, 11 Apr 2020, Andreas Rheinhardt wrote: > The documentation of av_write_frame() explicitly states that the function > doesn't take ownership of the packets sent to it; while av_write_frame() > does not directly unreference the packets after having written them, it > nevertheless modifies the packet in various ways: > 1. The timestamps might be modified either by prepare_input_packet() or > compute_muxer_pkt_fields(). > 2. If a bitstream filter gets applied, it takes ownership of the > reference and the side-data in the packet sent to it. > In case of do_packet_auto_bsf(), the end result is that the returned packet > contains the output of the last bsf in the chain. If an error happens, > a blank packet will be returned; a packet may also simply not lead to > any output (vp9_superframe). > This also implies that side data needs to be really copied and can't be > shared with the input packet. > The method choosen here minimizes copying of data: When the input isn't > refcounted and no bitstream filter is applied, the packet's data will > not be copied. > > Notice that packets that contain uncoded frames are exempt from this > because these packets are not owned by and returned to the user. This > also moves unreferencing the packets containing uncoded frames to > av_write_frame() in the noninterleaved codepath; in the interleaved > codepath, these packets are already freed in av_interleaved_write_frame(), > so that unreferencing the packets in write_uncoded_frame_internal() is > no longer needed. It has been removed. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > I was actually surprised when I realized how freeing uncoded frames in > the noninterleaved codepath could be left to av_write_frame(). > > libavformat/mux.c | 48 +++++++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/libavformat/mux.c b/libavformat/mux.c > index cae9f42d11..48c0d4cd5f 100644 > --- a/libavformat/mux.c > +++ b/libavformat/mux.c > @@ -874,11 +874,12 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { > return 1; > } > > -int av_write_frame(AVFormatContext *s, AVPacket *pkt) > +int av_write_frame(AVFormatContext *s, AVPacket *in) > { > + AVPacket local_pkt, *pkt = &local_pkt; > int ret; > > - if (!pkt) { > + if (!in) { > if (s->oformat->flags & AVFMT_ALLOW_FLUSH) { > ret = s->oformat->write_packet(s, NULL); > flush_if_needed(s); > @@ -889,22 +890,49 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt) > return 1; > } > > + if (in->flags & AV_PKT_FLAG_UNCODED_FRAME) { > + pkt = in; > + } else { > + /* We don't own in, so we have to make sure not to modify it. > + * The following avoids copying in's data unnecessarily. > + * Copying side data is unavoidable as a bitstream filter > + * may change it, e.g. free it on errors. */ > + pkt->buf = NULL; > + pkt->data = in->data; > + pkt->size = in->size; > + ret = av_packet_copy_props(pkt, in); > + if (ret < 0) > + return ret; > + if (in->buf) { > + pkt->buf = av_buffer_ref(in->buf); > + if (!pkt->buf) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > + } > + } > + > ret = prepare_input_packet(s, pkt); > if (ret < 0) > - return ret; > + goto fail; > > ret = do_packet_auto_bsf(s, pkt); > if (ret <= 0) > - return ret; > + goto fail; > > #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX > ret = compute_muxer_pkt_fields(s, s->streams[pkt->stream_index], pkt); > > if (ret < 0 && !(s->oformat->flags & AVFMT_NOTIMESTAMPS)) > - return ret; > + goto fail; > #endif > > - return write_packet(s, pkt); > + ret = write_packet(s, pkt); > + > +fail: > + // Uncoded frames using the noninterleaved codepath are freed here This comment does not seem accurate. Pkt must always be unrefed here, not only for the uncoded_frames (where it happens to be == in), or I miss something? If not, then I'd just simply remove this comment. Otherwise looks good. I checked the other patches in the series they all look good as well. Thanks, Marton > + av_packet_unref(pkt); > + return ret; > } > > #define CHUNK_START 0x1000 > @@ -1319,7 +1347,6 @@ 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) { > @@ -1349,11 +1376,8 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, > pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME; > } > > - ret = interleaved ? av_interleaved_write_frame(s, pktp) : > - av_write_frame(s, pktp); > - if (pktp) > - av_packet_unref(pktp); > - return ret; > + return interleaved ? av_interleaved_write_frame(s, pktp) : > + av_write_frame(s, pktp); > } > > int av_write_uncoded_frame(AVFormatContext *s, int stream_index, > -- > 2.20.1 > > _______________________________________________ > 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".
Marton Balint: > > > On Sat, 11 Apr 2020, Andreas Rheinhardt wrote: > >> The documentation of av_write_frame() explicitly states that the function >> doesn't take ownership of the packets sent to it; while av_write_frame() >> does not directly unreference the packets after having written them, it >> nevertheless modifies the packet in various ways: >> 1. The timestamps might be modified either by prepare_input_packet() or >> compute_muxer_pkt_fields(). >> 2. If a bitstream filter gets applied, it takes ownership of the >> reference and the side-data in the packet sent to it. >> In case of do_packet_auto_bsf(), the end result is that the returned >> packet >> contains the output of the last bsf in the chain. If an error happens, >> a blank packet will be returned; a packet may also simply not lead to >> any output (vp9_superframe). >> This also implies that side data needs to be really copied and can't be >> shared with the input packet. >> The method choosen here minimizes copying of data: When the input isn't >> refcounted and no bitstream filter is applied, the packet's data will >> not be copied. >> >> Notice that packets that contain uncoded frames are exempt from this >> because these packets are not owned by and returned to the user. This >> also moves unreferencing the packets containing uncoded frames to >> av_write_frame() in the noninterleaved codepath; in the interleaved >> codepath, these packets are already freed in >> av_interleaved_write_frame(), >> so that unreferencing the packets in write_uncoded_frame_internal() is >> no longer needed. It has been removed. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> I was actually surprised when I realized how freeing uncoded frames in >> the noninterleaved codepath could be left to av_write_frame(). >> >> libavformat/mux.c | 48 +++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 36 insertions(+), 12 deletions(-) >> >> diff --git a/libavformat/mux.c b/libavformat/mux.c >> index cae9f42d11..48c0d4cd5f 100644 >> --- a/libavformat/mux.c >> +++ b/libavformat/mux.c >> @@ -874,11 +874,12 @@ static int do_packet_auto_bsf(AVFormatContext >> *s, AVPacket *pkt) { >> return 1; >> } >> >> -int av_write_frame(AVFormatContext *s, AVPacket *pkt) >> +int av_write_frame(AVFormatContext *s, AVPacket *in) >> { >> + AVPacket local_pkt, *pkt = &local_pkt; >> int ret; >> >> - if (!pkt) { >> + if (!in) { >> if (s->oformat->flags & AVFMT_ALLOW_FLUSH) { >> ret = s->oformat->write_packet(s, NULL); >> flush_if_needed(s); >> @@ -889,22 +890,49 @@ int av_write_frame(AVFormatContext *s, AVPacket >> *pkt) >> return 1; >> } >> >> + if (in->flags & AV_PKT_FLAG_UNCODED_FRAME) { >> + pkt = in; >> + } else { >> + /* We don't own in, so we have to make sure not to modify it. >> + * The following avoids copying in's data unnecessarily. >> + * Copying side data is unavoidable as a bitstream filter >> + * may change it, e.g. free it on errors. */ >> + pkt->buf = NULL; >> + pkt->data = in->data; >> + pkt->size = in->size; >> + ret = av_packet_copy_props(pkt, in); >> + if (ret < 0) >> + return ret; >> + if (in->buf) { >> + pkt->buf = av_buffer_ref(in->buf); >> + if (!pkt->buf) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> + } >> + } >> + >> ret = prepare_input_packet(s, pkt); >> if (ret < 0) >> - return ret; >> + goto fail; >> >> ret = do_packet_auto_bsf(s, pkt); >> if (ret <= 0) >> - return ret; >> + goto fail; >> >> #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX >> ret = compute_muxer_pkt_fields(s, s->streams[pkt->stream_index], >> pkt); >> >> if (ret < 0 && !(s->oformat->flags & AVFMT_NOTIMESTAMPS)) >> - return ret; >> + goto fail; >> #endif >> >> - return write_packet(s, pkt); >> + ret = write_packet(s, pkt); >> + >> +fail: >> + // Uncoded frames using the noninterleaved codepath are freed here > > This comment does not seem accurate. Pkt must always be unrefed here, > not only for the uncoded_frames (where it happens to be == in), or I > miss something? If not, then I'd just simply remove this comment. Of course ordinary packets need to be unreferenced here, too; but I thought that someone reading and potentially changing av_write_frame() is already aware of that. But they might not be aware of the fact that (contrary to the usual behaviour of av_write_frame()) some packets not created in av_write_frame() are unreferenced there, hence this comment. - Andreas
On Thu, 16 Apr 2020, Andreas Rheinhardt wrote: > Marton Balint: >> >> >> On Sat, 11 Apr 2020, Andreas Rheinhardt wrote: >> >>> The documentation of av_write_frame() explicitly states that the function >>> doesn't take ownership of the packets sent to it; while av_write_frame() >>> does not directly unreference the packets after having written them, it >>> nevertheless modifies the packet in various ways: >>> 1. The timestamps might be modified either by prepare_input_packet() or >>> compute_muxer_pkt_fields(). >>> 2. If a bitstream filter gets applied, it takes ownership of the >>> reference and the side-data in the packet sent to it. >>> In case of do_packet_auto_bsf(), the end result is that the returned >>> packet >>> contains the output of the last bsf in the chain. If an error happens, >>> a blank packet will be returned; a packet may also simply not lead to >>> any output (vp9_superframe). >>> This also implies that side data needs to be really copied and can't be >>> shared with the input packet. >>> The method choosen here minimizes copying of data: When the input isn't >>> refcounted and no bitstream filter is applied, the packet's data will >>> not be copied. >>> >>> Notice that packets that contain uncoded frames are exempt from this >>> because these packets are not owned by and returned to the user. This >>> also moves unreferencing the packets containing uncoded frames to >>> av_write_frame() in the noninterleaved codepath; in the interleaved >>> codepath, these packets are already freed in >>> av_interleaved_write_frame(), >>> so that unreferencing the packets in write_uncoded_frame_internal() is >>> no longer needed. It has been removed. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> I was actually surprised when I realized how freeing uncoded frames in >>> the noninterleaved codepath could be left to av_write_frame(). >>> >>> libavformat/mux.c | 48 +++++++++++++++++++++++++++++++++++------------ >>> 1 file changed, 36 insertions(+), 12 deletions(-) >>> >>> diff --git a/libavformat/mux.c b/libavformat/mux.c >>> index cae9f42d11..48c0d4cd5f 100644 >>> --- a/libavformat/mux.c >>> +++ b/libavformat/mux.c >>> @@ -874,11 +874,12 @@ static int do_packet_auto_bsf(AVFormatContext >>> *s, AVPacket *pkt) { >>> return 1; >>> } >>> >>> -int av_write_frame(AVFormatContext *s, AVPacket *pkt) >>> +int av_write_frame(AVFormatContext *s, AVPacket *in) >>> { >>> + AVPacket local_pkt, *pkt = &local_pkt; >>> int ret; >>> >>> - if (!pkt) { >>> + if (!in) { >>> if (s->oformat->flags & AVFMT_ALLOW_FLUSH) { >>> ret = s->oformat->write_packet(s, NULL); >>> flush_if_needed(s); >>> @@ -889,22 +890,49 @@ int av_write_frame(AVFormatContext *s, AVPacket >>> *pkt) >>> return 1; >>> } >>> >>> + if (in->flags & AV_PKT_FLAG_UNCODED_FRAME) { >>> + pkt = in; >>> + } else { >>> + /* We don't own in, so we have to make sure not to modify it. >>> + * The following avoids copying in's data unnecessarily. >>> + * Copying side data is unavoidable as a bitstream filter >>> + * may change it, e.g. free it on errors. */ >>> + pkt->buf = NULL; >>> + pkt->data = in->data; >>> + pkt->size = in->size; >>> + ret = av_packet_copy_props(pkt, in); >>> + if (ret < 0) >>> + return ret; >>> + if (in->buf) { >>> + pkt->buf = av_buffer_ref(in->buf); >>> + if (!pkt->buf) { >>> + ret = AVERROR(ENOMEM); >>> + goto fail; >>> + } >>> + } >>> + } >>> + >>> ret = prepare_input_packet(s, pkt); >>> if (ret < 0) >>> - return ret; >>> + goto fail; >>> >>> ret = do_packet_auto_bsf(s, pkt); >>> if (ret <= 0) >>> - return ret; >>> + goto fail; >>> >>> #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX >>> ret = compute_muxer_pkt_fields(s, s->streams[pkt->stream_index], >>> pkt); >>> >>> if (ret < 0 && !(s->oformat->flags & AVFMT_NOTIMESTAMPS)) >>> - return ret; >>> + goto fail; >>> #endif >>> >>> - return write_packet(s, pkt); >>> + ret = write_packet(s, pkt); >>> + >>> +fail: >>> + // Uncoded frames using the noninterleaved codepath are freed here >> >> This comment does not seem accurate. Pkt must always be unrefed here, >> not only for the uncoded_frames (where it happens to be == in), or I >> miss something? If not, then I'd just simply remove this comment. > > Of course ordinary packets need to be unreferenced here, too; but I > thought that someone reading and potentially changing av_write_frame() > is already aware of that. But they might not be aware of the fact that > (contrary to the usual behaviour of av_write_frame()) some packets not > created in av_write_frame() are unreferenced there, hence this comment. Wow, that really confused me :) Then write something like: uncoded frames are *also* freed here. Thanks, Marton
diff --git a/libavformat/mux.c b/libavformat/mux.c index cae9f42d11..48c0d4cd5f 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -874,11 +874,12 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { return 1; } -int av_write_frame(AVFormatContext *s, AVPacket *pkt) +int av_write_frame(AVFormatContext *s, AVPacket *in) { + AVPacket local_pkt, *pkt = &local_pkt; int ret; - if (!pkt) { + if (!in) { if (s->oformat->flags & AVFMT_ALLOW_FLUSH) { ret = s->oformat->write_packet(s, NULL); flush_if_needed(s); @@ -889,22 +890,49 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt) return 1; } + if (in->flags & AV_PKT_FLAG_UNCODED_FRAME) { + pkt = in; + } else { + /* We don't own in, so we have to make sure not to modify it. + * The following avoids copying in's data unnecessarily. + * Copying side data is unavoidable as a bitstream filter + * may change it, e.g. free it on errors. */ + pkt->buf = NULL; + pkt->data = in->data; + pkt->size = in->size; + ret = av_packet_copy_props(pkt, in); + if (ret < 0) + return ret; + if (in->buf) { + pkt->buf = av_buffer_ref(in->buf); + if (!pkt->buf) { + ret = AVERROR(ENOMEM); + goto fail; + } + } + } + ret = prepare_input_packet(s, pkt); if (ret < 0) - return ret; + goto fail; ret = do_packet_auto_bsf(s, pkt); if (ret <= 0) - return ret; + goto fail; #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX ret = compute_muxer_pkt_fields(s, s->streams[pkt->stream_index], pkt); if (ret < 0 && !(s->oformat->flags & AVFMT_NOTIMESTAMPS)) - return ret; + goto fail; #endif - return write_packet(s, pkt); + ret = write_packet(s, pkt); + +fail: + // Uncoded frames using the noninterleaved codepath are freed here + av_packet_unref(pkt); + return ret; } #define CHUNK_START 0x1000 @@ -1319,7 +1347,6 @@ 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) { @@ -1349,11 +1376,8 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME; } - ret = interleaved ? av_interleaved_write_frame(s, pktp) : - av_write_frame(s, pktp); - if (pktp) - av_packet_unref(pktp); - return ret; + return interleaved ? av_interleaved_write_frame(s, pktp) : + av_write_frame(s, pktp); } int av_write_uncoded_frame(AVFormatContext *s, int stream_index,
The documentation of av_write_frame() explicitly states that the function doesn't take ownership of the packets sent to it; while av_write_frame() does not directly unreference the packets after having written them, it nevertheless modifies the packet in various ways: 1. The timestamps might be modified either by prepare_input_packet() or compute_muxer_pkt_fields(). 2. If a bitstream filter gets applied, it takes ownership of the reference and the side-data in the packet sent to it. In case of do_packet_auto_bsf(), the end result is that the returned packet contains the output of the last bsf in the chain. If an error happens, a blank packet will be returned; a packet may also simply not lead to any output (vp9_superframe). This also implies that side data needs to be really copied and can't be shared with the input packet. The method choosen here minimizes copying of data: When the input isn't refcounted and no bitstream filter is applied, the packet's data will not be copied. Notice that packets that contain uncoded frames are exempt from this because these packets are not owned by and returned to the user. This also moves unreferencing the packets containing uncoded frames to av_write_frame() in the noninterleaved codepath; in the interleaved codepath, these packets are already freed in av_interleaved_write_frame(), so that unreferencing the packets in write_uncoded_frame_internal() is no longer needed. It has been removed. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- I was actually surprised when I realized how freeing uncoded frames in the noninterleaved codepath could be left to av_write_frame(). libavformat/mux.c | 48 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-)