diff mbox series

[FFmpeg-devel,2/3] avfilter/src_movie: Remove unnecessary secondary AVPacket

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 10, 2020, 1:39 a.m. UTC
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(-)

Comments

Nicolas George Sept. 10, 2020, 8:08 a.m. UTC | #1
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,
Andreas Rheinhardt Sept. 10, 2020, 12:05 p.m. UTC | #2
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 mbox series

Patch

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;