| Message ID | 20210204191005.48190-24-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | deprecate av_init_packet() and sizeof(AVPacket) as part of the ABI | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
James Almer: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/internal.h | 5 +++++ > libavformat/mux.c | 44 ++++++++++++++++++++++-------------------- > libavformat/options.c | 6 ++++++ > libavformat/utils.c | 1 + > 4 files changed, 35 insertions(+), 21 deletions(-) > > diff --git a/libavformat/internal.h b/libavformat/internal.h > index d0db331b96..69a7caff93 100644 > --- a/libavformat/internal.h > +++ b/libavformat/internal.h > @@ -92,6 +92,11 @@ struct AVFormatInternal { > */ > struct AVPacketList *parse_queue; > struct AVPacketList *parse_queue_end; > + > + /** > + * Used to hold temporary packets. > + */ > + AVPacket *pkt; > /** > * Remaining size available for raw_packet_buffer, in bytes. > */ > diff --git a/libavformat/mux.c b/libavformat/mux.c > index 84c56ac6ba..3600e74a50 100644 > --- a/libavformat/mux.c > +++ b/libavformat/mux.c > @@ -1052,7 +1052,9 @@ int ff_interleaved_peek(AVFormatContext *s, int stream, > AVPacketList *pktl = s->internal->packet_buffer; > while (pktl) { > if (pktl->pkt.stream_index == stream) { > - *pkt = pktl->pkt; > + int ret = av_packet_ref(pkt, &pktl->pkt); > + if (ret < 0) > + return ret; This will lead to memleaks, because up until now the callers just received a non-ownership packets, ergo they did not unref the packet. (The fate-movenc test fails with this when run under valgrind/asan.) Returning a pointer to a const AVPacket and signaling the offsetted timestamps via other pointer arguments would avoid this problem. > if (add_offset) { > AVStream *st = s->streams[pkt->stream_index]; > int64_t offset = st->internal->mux_ts_offset; > @@ -1208,7 +1210,7 @@ static int write_packets_common(AVFormatContext *s, AVPacket *pkt, int interleav > > int av_write_frame(AVFormatContext *s, AVPacket *in) > { > - AVPacket local_pkt, *pkt = &local_pkt; > + AVPacket *pkt = s->internal->pkt; > int ret; > > if (!in) { > @@ -1229,6 +1231,7 @@ int av_write_frame(AVFormatContext *s, AVPacket *in) > * 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. */ > + av_packet_unref(pkt); > pkt->buf = NULL; > pkt->data = in->data; > pkt->size = in->size; > @@ -1270,14 +1273,14 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt) > int av_write_trailer(AVFormatContext *s) > { > int i, ret1, ret = 0; > - AVPacket pkt = {0}; > - av_init_packet(&pkt); > + AVPacket *pkt = s->internal->pkt; > > + av_packet_unref(pkt); > for (i = 0; i < s->nb_streams; i++) { > if (s->streams[i]->internal->bsfc) { > - ret1 = write_packets_from_bsfs(s, s->streams[i], &pkt, 1/*interleaved*/); > + ret1 = write_packets_from_bsfs(s, s->streams[i], pkt, 1/*interleaved*/); > if (ret1 < 0) > - av_packet_unref(&pkt); > + av_packet_unref(pkt); > if (ret >= 0) > ret = ret1; > } > @@ -1351,7 +1354,7 @@ static void uncoded_frame_free(void *unused, uint8_t *data) > static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, > AVFrame *frame, int interleaved) > { > - AVPacket pkt, *pktp; > + AVPacket *pkt = s->internal->pkt; > > av_assert0(s->oformat); > if (!s->oformat->write_uncoded_frame) { > @@ -1360,18 +1363,17 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, > } > > if (!frame) { > - pktp = NULL; > + pkt = 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.buf = av_buffer_create((void *)framep, bufsize, > + av_packet_unref(pkt); > + pkt->buf = av_buffer_create((void *)framep, bufsize, > uncoded_frame_free, NULL, 0); > - if (!pkt.buf) { > + if (!pkt->buf) { > av_free(framep); > fail: > av_frame_free(&frame); > @@ -1379,17 +1381,17 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, > } > *framep = frame; > > - pkt.data = (void *)framep; > - pkt.size = sizeof(frame); > - pkt.pts = > - pkt.dts = frame->pts; > - pkt.duration = frame->pkt_duration; > - pkt.stream_index = stream_index; > - pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME; > + pkt->data = (void *)framep; > + pkt->size = sizeof(frame); > + pkt->pts = > + pkt->dts = frame->pts; > + pkt->duration = frame->pkt_duration; > + pkt->stream_index = stream_index; > + pkt->flags |= AV_PKT_FLAG_UNCODED_FRAME; > } > > - return interleaved ? av_interleaved_write_frame(s, pktp) : > - av_write_frame(s, pktp); > + return interleaved ? av_interleaved_write_frame(s, pkt) : > + av_write_frame(s, pkt); > } > > int av_write_uncoded_frame(AVFormatContext *s, int stream_index, > diff --git a/libavformat/options.c b/libavformat/options.c > index 59e0389815..8d7c4fe4cb 100644 > --- a/libavformat/options.c > +++ b/libavformat/options.c > @@ -220,6 +220,12 @@ AVFormatContext *avformat_alloc_context(void) > av_free(ic); > return NULL; > } > + internal->pkt = av_packet_alloc(); > + if (!internal->pkt) { > + av_free(internal); > + av_free(ic); > + return NULL; > + } > avformat_get_context_defaults(ic); > ic->internal = internal; > ic->internal->offset = AV_NOPTS_VALUE; > diff --git a/libavformat/utils.c b/libavformat/utils.c > index fb3299503e..2587bedc05 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -4445,6 +4445,7 @@ void avformat_free_context(AVFormatContext *s) > av_freep(&s->chapters); > av_dict_free(&s->metadata); > av_dict_free(&s->internal->id3v2_meta); > + av_packet_free(&s->internal->pkt); > av_freep(&s->streams); > flush_packet_queue(s); > av_freep(&s->internal); >
On 2/8/2021 3:22 PM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer<jamrial@gmail.com> >> --- >> libavformat/internal.h | 5 +++++ >> libavformat/mux.c | 44 ++++++++++++++++++++++-------------------- >> libavformat/options.c | 6 ++++++ >> libavformat/utils.c | 1 + >> 4 files changed, 35 insertions(+), 21 deletions(-) >> >> diff --git a/libavformat/internal.h b/libavformat/internal.h >> index d0db331b96..69a7caff93 100644 >> --- a/libavformat/internal.h >> +++ b/libavformat/internal.h >> @@ -92,6 +92,11 @@ struct AVFormatInternal { >> */ >> struct AVPacketList *parse_queue; >> struct AVPacketList *parse_queue_end; >> + >> + /** >> + * Used to hold temporary packets. >> + */ >> + AVPacket *pkt; >> /** >> * Remaining size available for raw_packet_buffer, in bytes. >> */ >> diff --git a/libavformat/mux.c b/libavformat/mux.c >> index 84c56ac6ba..3600e74a50 100644 >> --- a/libavformat/mux.c >> +++ b/libavformat/mux.c >> @@ -1052,7 +1052,9 @@ int ff_interleaved_peek(AVFormatContext *s, int stream, >> AVPacketList *pktl = s->internal->packet_buffer; >> while (pktl) { >> if (pktl->pkt.stream_index == stream) { >> - *pkt = pktl->pkt; >> + int ret = av_packet_ref(pkt, &pktl->pkt); >> + if (ret < 0) >> + return ret; > This will lead to memleaks, because up until now the callers just > received a non-ownership packets, ergo they did not unref the packet. > (The fate-movenc test fails with this when run under valgrind/asan.) > > Returning a pointer to a const AVPacket and signaling the offsetted > timestamps via other pointer arguments would avoid this problem. There's a single user of ff_interleaved_peek(), which is the movenc one, and i made it unref the packet right after using it. But that's done in the following patch, so i guess i should change this function and its one caller at the same time. Also, i could just return the offsetted pts and dts values and not bother at all with packets. No allocations that way.
On 2/8/2021 3:50 PM, James Almer wrote: > On 2/8/2021 3:22 PM, Andreas Rheinhardt wrote: >> James Almer: >>> Signed-off-by: James Almer<jamrial@gmail.com> >>> --- >>> libavformat/internal.h | 5 +++++ >>> libavformat/mux.c | 44 ++++++++++++++++++++++-------------------- >>> libavformat/options.c | 6 ++++++ >>> libavformat/utils.c | 1 + >>> 4 files changed, 35 insertions(+), 21 deletions(-) >>> >>> diff --git a/libavformat/internal.h b/libavformat/internal.h >>> index d0db331b96..69a7caff93 100644 >>> --- a/libavformat/internal.h >>> +++ b/libavformat/internal.h >>> @@ -92,6 +92,11 @@ struct AVFormatInternal { >>> */ >>> struct AVPacketList *parse_queue; >>> struct AVPacketList *parse_queue_end; >>> + >>> + /** >>> + * Used to hold temporary packets. >>> + */ >>> + AVPacket *pkt; >>> /** >>> * Remaining size available for raw_packet_buffer, in bytes. >>> */ >>> diff --git a/libavformat/mux.c b/libavformat/mux.c >>> index 84c56ac6ba..3600e74a50 100644 >>> --- a/libavformat/mux.c >>> +++ b/libavformat/mux.c >>> @@ -1052,7 +1052,9 @@ int ff_interleaved_peek(AVFormatContext *s, int >>> stream, >>> AVPacketList *pktl = s->internal->packet_buffer; >>> while (pktl) { >>> if (pktl->pkt.stream_index == stream) { >>> - *pkt = pktl->pkt; >>> + int ret = av_packet_ref(pkt, &pktl->pkt); >>> + if (ret < 0) >>> + return ret; >> This will lead to memleaks, because up until now the callers just >> received a non-ownership packets, ergo they did not unref the packet. >> (The fate-movenc test fails with this when run under valgrind/asan.) >> >> Returning a pointer to a const AVPacket and signaling the offsetted >> timestamps via other pointer arguments would avoid this problem. > > There's a single user of ff_interleaved_peek(), which is the movenc one, > and i made it unref the packet right after using it. But that's done in > the following patch, so i guess i should change this function and its > one caller at the same time. > > Also, i could just return the offsetted pts and dts values and not > bother at all with packets. No allocations that way. Decided to implement your idea with a few changes in a separate patchset.
diff --git a/libavformat/internal.h b/libavformat/internal.h index d0db331b96..69a7caff93 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -92,6 +92,11 @@ struct AVFormatInternal { */ struct AVPacketList *parse_queue; struct AVPacketList *parse_queue_end; + + /** + * Used to hold temporary packets. + */ + AVPacket *pkt; /** * Remaining size available for raw_packet_buffer, in bytes. */ diff --git a/libavformat/mux.c b/libavformat/mux.c index 84c56ac6ba..3600e74a50 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -1052,7 +1052,9 @@ int ff_interleaved_peek(AVFormatContext *s, int stream, AVPacketList *pktl = s->internal->packet_buffer; while (pktl) { if (pktl->pkt.stream_index == stream) { - *pkt = pktl->pkt; + int ret = av_packet_ref(pkt, &pktl->pkt); + if (ret < 0) + return ret; if (add_offset) { AVStream *st = s->streams[pkt->stream_index]; int64_t offset = st->internal->mux_ts_offset; @@ -1208,7 +1210,7 @@ static int write_packets_common(AVFormatContext *s, AVPacket *pkt, int interleav int av_write_frame(AVFormatContext *s, AVPacket *in) { - AVPacket local_pkt, *pkt = &local_pkt; + AVPacket *pkt = s->internal->pkt; int ret; if (!in) { @@ -1229,6 +1231,7 @@ int av_write_frame(AVFormatContext *s, AVPacket *in) * 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. */ + av_packet_unref(pkt); pkt->buf = NULL; pkt->data = in->data; pkt->size = in->size; @@ -1270,14 +1273,14 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt) int av_write_trailer(AVFormatContext *s) { int i, ret1, ret = 0; - AVPacket pkt = {0}; - av_init_packet(&pkt); + AVPacket *pkt = s->internal->pkt; + av_packet_unref(pkt); for (i = 0; i < s->nb_streams; i++) { if (s->streams[i]->internal->bsfc) { - ret1 = write_packets_from_bsfs(s, s->streams[i], &pkt, 1/*interleaved*/); + ret1 = write_packets_from_bsfs(s, s->streams[i], pkt, 1/*interleaved*/); if (ret1 < 0) - av_packet_unref(&pkt); + av_packet_unref(pkt); if (ret >= 0) ret = ret1; } @@ -1351,7 +1354,7 @@ static void uncoded_frame_free(void *unused, uint8_t *data) static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, AVFrame *frame, int interleaved) { - AVPacket pkt, *pktp; + AVPacket *pkt = s->internal->pkt; av_assert0(s->oformat); if (!s->oformat->write_uncoded_frame) { @@ -1360,18 +1363,17 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, } if (!frame) { - pktp = NULL; + pkt = 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.buf = av_buffer_create((void *)framep, bufsize, + av_packet_unref(pkt); + pkt->buf = av_buffer_create((void *)framep, bufsize, uncoded_frame_free, NULL, 0); - if (!pkt.buf) { + if (!pkt->buf) { av_free(framep); fail: av_frame_free(&frame); @@ -1379,17 +1381,17 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, } *framep = frame; - pkt.data = (void *)framep; - pkt.size = sizeof(frame); - pkt.pts = - pkt.dts = frame->pts; - pkt.duration = frame->pkt_duration; - pkt.stream_index = stream_index; - pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME; + pkt->data = (void *)framep; + pkt->size = sizeof(frame); + pkt->pts = + pkt->dts = frame->pts; + pkt->duration = frame->pkt_duration; + pkt->stream_index = stream_index; + pkt->flags |= AV_PKT_FLAG_UNCODED_FRAME; } - return interleaved ? av_interleaved_write_frame(s, pktp) : - av_write_frame(s, pktp); + return interleaved ? av_interleaved_write_frame(s, pkt) : + av_write_frame(s, pkt); } int av_write_uncoded_frame(AVFormatContext *s, int stream_index, diff --git a/libavformat/options.c b/libavformat/options.c index 59e0389815..8d7c4fe4cb 100644 --- a/libavformat/options.c +++ b/libavformat/options.c @@ -220,6 +220,12 @@ AVFormatContext *avformat_alloc_context(void) av_free(ic); return NULL; } + internal->pkt = av_packet_alloc(); + if (!internal->pkt) { + av_free(internal); + av_free(ic); + return NULL; + } avformat_get_context_defaults(ic); ic->internal = internal; ic->internal->offset = AV_NOPTS_VALUE; diff --git a/libavformat/utils.c b/libavformat/utils.c index fb3299503e..2587bedc05 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -4445,6 +4445,7 @@ void avformat_free_context(AVFormatContext *s) av_freep(&s->chapters); av_dict_free(&s->metadata); av_dict_free(&s->internal->id3v2_meta); + av_packet_free(&s->internal->pkt); av_freep(&s->streams); flush_packet_queue(s); av_freep(&s->internal);
Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/internal.h | 5 +++++ libavformat/mux.c | 44 ++++++++++++++++++++++-------------------- libavformat/options.c | 6 ++++++ libavformat/utils.c | 1 + 4 files changed, 35 insertions(+), 21 deletions(-)