| Message ID | 20200910013941.14444-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 41e0058b48fc192aaa7b97a58636f4784b93b6fd |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avfilter/src_movie: Remove unneeded resetting of AVPacket | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Andreas Rheinhardt (12020-09-10): > The movie and amovie filters currently use two packets. One of the two, > pkt0, is the owner of the returned packet; it is also the destination > packet for av_read_frame(). The other one pkt is initially (i.e. after > av_read_frame()) a copy of pkt0; copy means that the contents of both > are absolutely the same: They both point to the same AVBufferRef and the > same side data. This violation of the refcounted packet API is only > possible because pkt is not considered to own its data. Only pkt0 is > ever unreferenced. > The reason for pkt's existence seems to be historic: > The API used for decoding audio (namely avcodec_decode_audio4()) could > consume frames partially, i.e. it could return multiple frames for one > packet and therefore it returned how much of the input buffer had been > consumed. The caller was then supposed to update the packet's data and > size pointer to reflect this and call avcodec_decode_audio4() again with > the updated packet to get the next frame. > But before the introduction of refcounted AVPackets where knowledge and > responsibility about what to free lies with the underlying AVBuffer such > a procedure required a spare packet (or one would need to record the > original data and size fields separately to restore them before freeing > the packet; notice that this code has been written when AVPackets still > had a destruct field). But these times are long gone, so just remove the > secondary AVPacket. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > It seems that the avcodec_decode_audio4() lost the ability to output > multiple frames per packet in 061a0c14bb5767bca72e3a7227ca400de439ba09. > Am I right? I think I am ok with the change (and the two others), but I think it would be much more useful update src_movie to use the new decoding API: no need to keep a packet in the context, no need to distinguish between audio and video. More work, but more useful. And it would allow to properly implement an activate version. Regards,
Nicolas George: > Andreas Rheinhardt (12020-09-10): >> The movie and amovie filters currently use two packets. One of the two, >> pkt0, is the owner of the returned packet; it is also the destination >> packet for av_read_frame(). The other one pkt is initially (i.e. after >> av_read_frame()) a copy of pkt0; copy means that the contents of both >> are absolutely the same: They both point to the same AVBufferRef and the >> same side data. This violation of the refcounted packet API is only >> possible because pkt is not considered to own its data. Only pkt0 is >> ever unreferenced. >> The reason for pkt's existence seems to be historic: >> The API used for decoding audio (namely avcodec_decode_audio4()) could >> consume frames partially, i.e. it could return multiple frames for one >> packet and therefore it returned how much of the input buffer had been >> consumed. The caller was then supposed to update the packet's data and >> size pointer to reflect this and call avcodec_decode_audio4() again with >> the updated packet to get the next frame. >> But before the introduction of refcounted AVPackets where knowledge and >> responsibility about what to free lies with the underlying AVBuffer such >> a procedure required a spare packet (or one would need to record the >> original data and size fields separately to restore them before freeing >> the packet; notice that this code has been written when AVPackets still >> had a destruct field). But these times are long gone, so just remove the >> secondary AVPacket. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> It seems that the avcodec_decode_audio4() lost the ability to output >> multiple frames per packet in 061a0c14bb5767bca72e3a7227ca400de439ba09. >> Am I right? > > I think I am ok with the change (and the two others), but I think it > would be much more useful update src_movie to use the new decoding API: > no need to keep a packet in the context, no need to distinguish between > audio and video. > > More work, but more useful. And it would allow to properly implement an > activate version. > > Regards, > I might implement the switch to the new decoding API later, but I wanted to fix the memleak first. - Andreas
diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c index b9b940684d..75ac3bfaf6 100644 --- a/libavfilter/src_movie.c +++ b/libavfilter/src_movie.c @@ -71,7 +71,7 @@ typedef struct MovieContext { AVFormatContext *format_ctx; int eof; - AVPacket pkt, pkt0; + AVPacket pkt; int max_stream_index; /**< max stream # actually used for output */ MovieStream *st; /**< array of all streams, one per output */ @@ -493,25 +493,21 @@ static int movie_push_frame(AVFilterContext *ctx, unsigned out_id) pkt->stream_index = movie->st[out_id].st->index; /* packet is already ready for flushing */ } else { - ret = av_read_frame(movie->format_ctx, &movie->pkt0); + ret = av_read_frame(movie->format_ctx, pkt); if (ret < 0) { - *pkt = movie->pkt0; if (ret == AVERROR_EOF) { movie->eof = 1; return 0; /* start flushing */ } return ret; } - *pkt = movie->pkt0; } } pkt_out_id = pkt->stream_index > movie->max_stream_index ? -1 : movie->out_index[pkt->stream_index]; if (pkt_out_id < 0) { - av_packet_unref(&movie->pkt0); - pkt->size = 0; /* ready for next run */ - pkt->data = NULL; + av_packet_unref(pkt); return 0; } st = &movie->st[pkt_out_id]; @@ -536,9 +532,7 @@ static int movie_push_frame(AVFilterContext *ctx, unsigned out_id) if (ret < 0) { av_log(ctx, AV_LOG_WARNING, "Decode error: %s\n", av_err2str(ret)); av_frame_free(&frame); - av_packet_unref(&movie->pkt0); - movie->pkt.size = 0; - movie->pkt.data = NULL; + av_packet_unref(pkt); return 0; } if (!ret || st->st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) @@ -546,11 +540,8 @@ static int movie_push_frame(AVFilterContext *ctx, unsigned out_id) pkt->data += ret; pkt->size -= ret; - if (pkt->size <= 0) { - av_packet_unref(&movie->pkt0); - pkt->size = 0; /* ready for next run */ - pkt->data = NULL; - } + if (pkt->size <= 0) + av_packet_unref(pkt); if (!got_frame) { if (!ret) st->done = 1;
The movie and amovie filters currently use two packets. One of the two, pkt0, is the owner of the returned packet; it is also the destination packet for av_read_frame(). The other one pkt is initially (i.e. after av_read_frame()) a copy of pkt0; copy means that the contents of both are absolutely the same: They both point to the same AVBufferRef and the same side data. This violation of the refcounted packet API is only possible because pkt is not considered to own its data. Only pkt0 is ever unreferenced. The reason for pkt's existence seems to be historic: The API used for decoding audio (namely avcodec_decode_audio4()) could consume frames partially, i.e. it could return multiple frames for one packet and therefore it returned how much of the input buffer had been consumed. The caller was then supposed to update the packet's data and size pointer to reflect this and call avcodec_decode_audio4() again with the updated packet to get the next frame. But before the introduction of refcounted AVPackets where knowledge and responsibility about what to free lies with the underlying AVBuffer such a procedure required a spare packet (or one would need to record the original data and size fields separately to restore them before freeing the packet; notice that this code has been written when AVPackets still had a destruct field). But these times are long gone, so just remove the secondary AVPacket. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- It seems that the avcodec_decode_audio4() lost the ability to output multiple frames per packet in 061a0c14bb5767bca72e3a7227ca400de439ba09. Am I right? libavfilter/src_movie.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)