| Message ID | 20200331123745.6461-6-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: > 1. When an error happened in ff_interleave_add_packet() during > interleaving a packet for output, said packet would not be unreferenced > in ff_interleave_add_packet(), but would be zeroed in > av_interleaved_write_frame(), which results in a memleak. > 2. When no error happened in ff_interleave_add_packet() during > interleaving a packet for output, the input packet will already be blank > (as if av_packet_unref() had been applied to it), but it will > nevertheless be zeroed and initialized again. This is unnecessary. > > Given the possibility of using other functions for interleavement than > ff_interleave_packet_per_dts(), this commit sets and documents what > interleavement functions need to do: On success, the input packet (if > given) should be a blank packet on return. On failure, cleaning up will > be done by av_interleave_write_frame() (where the already existing code > for error handling can be reused). ff_audio_rechunk_interleave() has been > changed to abide by this. In order to keep these requirements > consistent, they are kept in one place that is referenced by the other > documentations. > > Updating the documentation incidentally also removed another reference > to AVPacket's destructor. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/audiointerleave.c | 1 + > libavformat/audiointerleave.h | 4 ++++ > libavformat/avformat.h | 1 + > libavformat/internal.h | 16 +++++++--------- > libavformat/mux.c | 30 +++++++++++------------------- > 5 files changed, 24 insertions(+), 28 deletions(-) > > diff --git a/libavformat/audiointerleave.c b/libavformat/audiointerleave.c > index f10c83d7c9..d3b35d804e 100644 > --- a/libavformat/audiointerleave.c > +++ b/libavformat/audiointerleave.c > @@ -121,6 +121,7 @@ int ff_audio_rechunk_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt > aic->fifo_size = new_size; > } > av_fifo_generic_write(aic->fifo, pkt->data, pkt->size, NULL); > + av_packet_unref(pkt); This is a little bit suspicous that you are changing the requirements of the interleave_packet functions to always free its packets. This was not a requirement until now, the semantics of the muxer callbacks can be considered API. Yeah, probably has little impact, but still... > } else { > // rewrite pts and dts to be decoded time line position > pkt->pts = pkt->dts = aic->dts; > diff --git a/libavformat/audiointerleave.h b/libavformat/audiointerleave.h > index 0933310f4c..d21647b6b1 100644 > --- a/libavformat/audiointerleave.h > +++ b/libavformat/audiointerleave.h > @@ -48,6 +48,10 @@ void ff_audio_interleave_close(AVFormatContext *s); > * > * @param get_packet function will output a packet when streams are correctly interleaved. > * @param compare_ts function will compare AVPackets and decide interleaving order. > + * > + * Apart from these two functions, this function behaves like ff_interleave_packet_per_dts. > + * > + * @note This function must not be used with uncoded audio frames. > */ > int ff_audio_rechunk_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush, > int (*get_packet)(AVFormatContext *, AVPacket *, AVPacket *, int), > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 39b99b4481..8f7466931a 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -553,6 +553,7 @@ typedef struct AVOutputFormat { > /** > * A format-specific function for interleavement. > * If unset, packets will be interleaved by dts. > + * See the description of ff_interleave_packet_per_dts for details. > */ > int (*interleave_packet)(struct AVFormatContext *, AVPacket *out, > AVPacket *in, int flush); > diff --git a/libavformat/internal.h b/libavformat/internal.h > index 332477a532..a861acdc2a 100644 > --- a/libavformat/internal.h > +++ b/libavformat/internal.h > @@ -230,9 +230,9 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase); > int ff_hex_to_data(uint8_t *data, const char *p); > > /** > - * Add packet to AVFormatContext->packet_buffer list, determining its > + * Add packet to an AVFormatContext's packet_buffer list, determining its > * interleaved position using compare() function argument. > - * @return 0, or < 0 on error > + * @return 0, or < 0 on error. On success, pkt will be blank (as if unreferenced). I'd lose the "as if" > */ > int ff_interleave_add_packet(AVFormatContext *s, AVPacket *pkt, > int (*compare)(AVFormatContext *, const AVPacket *, const AVPacket *)); > @@ -495,15 +495,13 @@ int ff_framehash_write_header(AVFormatContext *s); > int ff_read_packet(AVFormatContext *s, AVPacket *pkt); > > /** > - * Interleave a packet per dts in an output media file. > - * > - * Packets with pkt->destruct == av_destruct_packet will be freed inside this > - * function, so they cannot be used after it. Note that calling av_packet_unref() > - * on them is still safe. > + * Interleave an AVPacket per dts so it can be muxed. > * > - * @param s media file handle > + * @param s an AVFormatContext for output. in resp. out will be added to > + * resp. taken from its packet buffer. I don't really understand this. What is "resp." here? > * @param out the interleaved packet will be output here > - * @param pkt the input packet > + * @param in the input packet. If not NULL will be a blank packet > + * (as if unreferenced) on success. I'd lose the "as if" here as well. > * @param flush 1 if no further packets are available as input and all > * remaining packets should be output > * @return 1 if a packet was output, 0 if no packet could be output, > diff --git a/libavformat/mux.c b/libavformat/mux.c > index a0ebcec119..79731d3008 100644 > --- a/libavformat/mux.c > +++ b/libavformat/mux.c > @@ -1166,20 +1166,13 @@ int ff_interleaved_peek(AVFormatContext *s, int stream, > > /** > * Interleave an AVPacket correctly so it can be muxed. > - * @param out the interleaved packet will be output here > - * @param in the input packet > - * @param flush 1 if no further packets are available as input and all > - * remaining packets should be output > - * @return 1 if a packet was output, 0 if no packet could be output, > - * < 0 if an error occurred > + * > + * See the description of ff_interleave_packet_per_dts for details. > */ > static int interleave_packet(AVFormatContext *s, AVPacket *out, AVPacket *in, int flush) > { > if (s->oformat->interleave_packet) { > - int ret = s->oformat->interleave_packet(s, out, in, flush); > - if (in) > - av_packet_unref(in); > - return ret; > + return s->oformat->interleave_packet(s, out, in, flush); If you want to keep this, and go ahead with the new expectations of the interleave_packet callback, then let's do an av_assert0 that in is a blank packet here on success. > } else > return ff_interleave_packet_per_dts(s, out, in, flush); > } > @@ -1222,14 +1215,13 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt) > > for (;; ) { > AVPacket opkt; > - int ret = interleave_packet(s, &opkt, pkt, flush); > - if (pkt) { > - memset(pkt, 0, sizeof(*pkt)); > - av_init_packet(pkt); > - pkt = NULL; > - } > - if (ret <= 0) //FIXME cleanup needed for ret<0 ? > - return ret; > + ret = interleave_packet(s, &opkt, pkt, flush); > + if (ret < 0) > + goto fail; > + if (!ret) > + return 0; > + > + pkt = NULL; > > ret = write_packet(s, &opkt); > > @@ -1242,7 +1234,7 @@ fail: > // 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)) > + if (pkt && !(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) > av_packet_unref(pkt); > return ret; > } > -- > 2.20.1 Regards, Marton
diff --git a/libavformat/audiointerleave.c b/libavformat/audiointerleave.c index f10c83d7c9..d3b35d804e 100644 --- a/libavformat/audiointerleave.c +++ b/libavformat/audiointerleave.c @@ -121,6 +121,7 @@ int ff_audio_rechunk_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt aic->fifo_size = new_size; } av_fifo_generic_write(aic->fifo, pkt->data, pkt->size, NULL); + av_packet_unref(pkt); } else { // rewrite pts and dts to be decoded time line position pkt->pts = pkt->dts = aic->dts; diff --git a/libavformat/audiointerleave.h b/libavformat/audiointerleave.h index 0933310f4c..d21647b6b1 100644 --- a/libavformat/audiointerleave.h +++ b/libavformat/audiointerleave.h @@ -48,6 +48,10 @@ void ff_audio_interleave_close(AVFormatContext *s); * * @param get_packet function will output a packet when streams are correctly interleaved. * @param compare_ts function will compare AVPackets and decide interleaving order. + * + * Apart from these two functions, this function behaves like ff_interleave_packet_per_dts. + * + * @note This function must not be used with uncoded audio frames. */ int ff_audio_rechunk_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush, int (*get_packet)(AVFormatContext *, AVPacket *, AVPacket *, int), diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 39b99b4481..8f7466931a 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -553,6 +553,7 @@ typedef struct AVOutputFormat { /** * A format-specific function for interleavement. * If unset, packets will be interleaved by dts. + * See the description of ff_interleave_packet_per_dts for details. */ int (*interleave_packet)(struct AVFormatContext *, AVPacket *out, AVPacket *in, int flush); diff --git a/libavformat/internal.h b/libavformat/internal.h index 332477a532..a861acdc2a 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -230,9 +230,9 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase); int ff_hex_to_data(uint8_t *data, const char *p); /** - * Add packet to AVFormatContext->packet_buffer list, determining its + * Add packet to an AVFormatContext's packet_buffer list, determining its * interleaved position using compare() function argument. - * @return 0, or < 0 on error + * @return 0, or < 0 on error. On success, pkt will be blank (as if unreferenced). */ int ff_interleave_add_packet(AVFormatContext *s, AVPacket *pkt, int (*compare)(AVFormatContext *, const AVPacket *, const AVPacket *)); @@ -495,15 +495,13 @@ int ff_framehash_write_header(AVFormatContext *s); int ff_read_packet(AVFormatContext *s, AVPacket *pkt); /** - * Interleave a packet per dts in an output media file. - * - * Packets with pkt->destruct == av_destruct_packet will be freed inside this - * function, so they cannot be used after it. Note that calling av_packet_unref() - * on them is still safe. + * Interleave an AVPacket per dts so it can be muxed. * - * @param s media file handle + * @param s an AVFormatContext for output. in resp. out will be added to + * resp. taken from its packet buffer. * @param out the interleaved packet will be output here - * @param pkt the input packet + * @param in the input packet. If not NULL will be a blank packet + * (as if unreferenced) on success. * @param flush 1 if no further packets are available as input and all * remaining packets should be output * @return 1 if a packet was output, 0 if no packet could be output, diff --git a/libavformat/mux.c b/libavformat/mux.c index a0ebcec119..79731d3008 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -1166,20 +1166,13 @@ int ff_interleaved_peek(AVFormatContext *s, int stream, /** * Interleave an AVPacket correctly so it can be muxed. - * @param out the interleaved packet will be output here - * @param in the input packet - * @param flush 1 if no further packets are available as input and all - * remaining packets should be output - * @return 1 if a packet was output, 0 if no packet could be output, - * < 0 if an error occurred + * + * See the description of ff_interleave_packet_per_dts for details. */ static int interleave_packet(AVFormatContext *s, AVPacket *out, AVPacket *in, int flush) { if (s->oformat->interleave_packet) { - int ret = s->oformat->interleave_packet(s, out, in, flush); - if (in) - av_packet_unref(in); - return ret; + return s->oformat->interleave_packet(s, out, in, flush); } else return ff_interleave_packet_per_dts(s, out, in, flush); } @@ -1222,14 +1215,13 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt) for (;; ) { AVPacket opkt; - int ret = interleave_packet(s, &opkt, pkt, flush); - if (pkt) { - memset(pkt, 0, sizeof(*pkt)); - av_init_packet(pkt); - pkt = NULL; - } - if (ret <= 0) //FIXME cleanup needed for ret<0 ? - return ret; + ret = interleave_packet(s, &opkt, pkt, flush); + if (ret < 0) + goto fail; + if (!ret) + return 0; + + pkt = NULL; ret = write_packet(s, &opkt); @@ -1242,7 +1234,7 @@ fail: // 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)) + if (pkt && !(pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) av_packet_unref(pkt); return ret; }
1. When an error happened in ff_interleave_add_packet() during interleaving a packet for output, said packet would not be unreferenced in ff_interleave_add_packet(), but would be zeroed in av_interleaved_write_frame(), which results in a memleak. 2. When no error happened in ff_interleave_add_packet() during interleaving a packet for output, the input packet will already be blank (as if av_packet_unref() had been applied to it), but it will nevertheless be zeroed and initialized again. This is unnecessary. Given the possibility of using other functions for interleavement than ff_interleave_packet_per_dts(), this commit sets and documents what interleavement functions need to do: On success, the input packet (if given) should be a blank packet on return. On failure, cleaning up will be done by av_interleave_write_frame() (where the already existing code for error handling can be reused). ff_audio_rechunk_interleave() has been changed to abide by this. In order to keep these requirements consistent, they are kept in one place that is referenced by the other documentations. Updating the documentation incidentally also removed another reference to AVPacket's destructor. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/audiointerleave.c | 1 + libavformat/audiointerleave.h | 4 ++++ libavformat/avformat.h | 1 + libavformat/internal.h | 16 +++++++--------- libavformat/mux.c | 30 +++++++++++------------------- 5 files changed, 24 insertions(+), 28 deletions(-)