diff mbox series

[FFmpeg-devel,1/4] avcodec: move avcodec_flush_buffers from decode.c to utils.c

Message ID 20200227180202.30982-2-jamrial@gmail.com
State New
Headers show
Series Encoding API code restructuration
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

James Almer Feb. 27, 2020, 6:01 p.m. UTC
It's not a decoding exclusive function.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/decode.c | 36 ------------------------------------
 libavcodec/utils.c  | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 36 deletions(-)

Comments

Anton Khirnov March 10, 2020, 6:03 p.m. UTC | #1
Quoting James Almer (2020-02-27 19:01:59)
> It's not a decoding exclusive function.

How come? The doxy for it says
'Reset the internal decoder state / flush internal buffers.'
James Almer March 10, 2020, 6:14 p.m. UTC | #2
On 3/10/2020 3:03 PM, Anton Khirnov wrote:
> Quoting James Almer (2020-02-27 19:01:59)
>> It's not a decoding exclusive function.
> 
> How come? The doxy for it says
> 'Reset the internal decoder state / flush internal buffers.'

It touches some fields used by encode.c, like draining and
buffer_pkt_valid (the latter which admittedly will be gone after this
patchset, but replaced by others), and there's at least one encoder
implementing an AVCodec.flush() callback.

And doesn't the slash in that doxy imply one or the other?
Anton Khirnov March 10, 2020, 7:08 p.m. UTC | #3
Quoting James Almer (2020-03-10 19:14:47)
> On 3/10/2020 3:03 PM, Anton Khirnov wrote:
> > Quoting James Almer (2020-02-27 19:01:59)
> >> It's not a decoding exclusive function.
> > 
> > How come? The doxy for it says
> > 'Reset the internal decoder state / flush internal buffers.'
> 
> It touches some fields used by encode.c, like draining and
> buffer_pkt_valid (the latter which admittedly will be gone after this
> patchset, but replaced by others),

draining is for decoders too I think

> and there's at least one encoder implementing an AVCodec.flush()
> callback.
> 
> And doesn't the slash in that doxy imply one or the other?

Hmm, apparently I wrote that doxy and have no recollection of it :)
I think it was supposed to be decoding only.

I see two encoders implementing flush: audiotoolbox and nvenc. The
addition to nvenc was explicit, but not even a year old, audiotoolbox
might or might not have been deliberate.

So it seems pretty unclear to me. I would say calling it on an encoder
is invalid and undefined, but apparently people are doing that and it
sort of sometimes works.
James Almer March 10, 2020, 7:33 p.m. UTC | #4
On 3/10/2020 4:08 PM, Anton Khirnov wrote:
> Quoting James Almer (2020-03-10 19:14:47)
>> On 3/10/2020 3:03 PM, Anton Khirnov wrote:
>>> Quoting James Almer (2020-02-27 19:01:59)
>>>> It's not a decoding exclusive function.
>>>
>>> How come? The doxy for it says
>>> 'Reset the internal decoder state / flush internal buffers.'
>>
>> It touches some fields used by encode.c, like draining and
>> buffer_pkt_valid (the latter which admittedly will be gone after this
>> patchset, but replaced by others),
> 
> draining is for decoders too I think
> 
>> and there's at least one encoder implementing an AVCodec.flush()
>> callback.
>>
>> And doesn't the slash in that doxy imply one or the other?
> 
> Hmm, apparently I wrote that doxy and have no recollection of it :)
> I think it was supposed to be decoding only.

I made the question because i interpreted it as such, and seeing
encoders adding flush callbacks, the same might have done others.
It wasn't the correct interpretation, so good to know.

> 
> I see two encoders implementing flush: audiotoolbox and nvenc. The
> addition to nvenc was explicit, but not even a year old, audiotoolbox
> might or might not have been deliberate.
> 
> So it seems pretty unclear to me. I would say calling it on an encoder
> is invalid and undefined, but apparently people are doing that and it
> sort of sometimes works.

I can withdraw this patch (And remove the relevant chunks from the
following two) if this function was effectively not meant for encoders.
Also maybe poke Timo regarding nvenc flushing, to see why it is needed
and if there's an alternative.
Philip Langdale April 16, 2020, 4:26 p.m. UTC | #5
On Tue, 10 Mar 2020 16:33:41 -0300
James Almer <jamrial@gmail.com> wrote:
> 
> I can withdraw this patch (And remove the relevant chunks from the
> following two) if this function was effectively not meant for
> encoders. Also maybe poke Timo regarding nvenc flushing, to see why
> it is needed and if there's an alternative.

I've pushed the change to formalise the encoder flush semantics, so
this should be good to apply now.

--phil
Philip Langdale April 17, 2020, 2:47 a.m. UTC | #6
On Thu, 16 Apr 2020 14:07:49 -0300
James Almer <jamrial@gmail.com> wrote:

> On 4/16/2020 1:26 PM, Philip Langdale wrote:
> > On Tue, 10 Mar 2020 16:33:41 -0300
> > James Almer <jamrial@gmail.com> wrote:  
> >>
> >> I can withdraw this patch (And remove the relevant chunks from the
> >> following two) if this function was effectively not meant for
> >> encoders. Also maybe poke Timo regarding nvenc flushing, to see why
> >> it is needed and if there's an alternative.  
> > 
> > I've pushed the change to formalise the encoder flush semantics, so
> > this should be good to apply now.  
> 
> All encoders were ported, so yes, it could be pushed. But i wonder if
> this should go in after 4.3 is tagged rather than now. I'd like this
> to have some months of testing in master before making it into a
> release.
> 
> Unless 4.3 is months away, in which case i'll just push it now.

Ok. You'll have a better sense of 4.3 timing than me, so I'll defer to
your judgement.

--phil
diff mbox series

Patch

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 03b9da25f9..93b0db7ef8 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -2020,42 +2020,6 @@  int ff_reget_buffer(AVCodecContext *avctx, AVFrame *frame, int flags)
     return ret;
 }
 
-static void bsfs_flush(AVCodecContext *avctx)
-{
-    DecodeFilterContext *s = &avctx->internal->filter;
-
-    for (int i = 0; i < s->nb_bsfs; i++)
-        av_bsf_flush(s->bsfs[i]);
-}
-
-void avcodec_flush_buffers(AVCodecContext *avctx)
-{
-    AVCodecInternal *avci = avctx->internal;
-
-    avci->draining      = 0;
-    avci->draining_done = 0;
-    avci->nb_draining_errors = 0;
-    av_frame_unref(avci->buffer_frame);
-    av_frame_unref(avci->compat_decode_frame);
-    av_packet_unref(avci->buffer_pkt);
-    avci->buffer_pkt_valid = 0;
-
-    av_packet_unref(avci->ds.in_pkt);
-
-    if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
-        ff_thread_flush(avctx);
-    else if (avctx->codec->flush)
-        avctx->codec->flush(avctx);
-
-    avctx->pts_correction_last_pts =
-    avctx->pts_correction_last_dts = INT64_MIN;
-
-    bsfs_flush(avctx);
-
-    if (!avctx->refcounted_frames)
-        av_frame_unref(avci->to_free);
-}
-
 void ff_decode_bsfs_uninit(AVCodecContext *avctx)
 {
     DecodeFilterContext *s = &avctx->internal->filter;
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index c4dc136d3c..5257a1c627 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -1084,6 +1084,39 @@  FF_ENABLE_DEPRECATION_WARNINGS
     goto end;
 }
 
+void avcodec_flush_buffers(AVCodecContext *avctx)
+{
+    AVCodecInternal *avci = avctx->internal;
+
+    avci->draining      = 0;
+    avci->draining_done = 0;
+    avci->nb_draining_errors = 0;
+    av_frame_unref(avci->buffer_frame);
+    av_frame_unref(avci->compat_decode_frame);
+    av_packet_unref(avci->buffer_pkt);
+    avci->buffer_pkt_valid = 0;
+
+    av_packet_unref(avci->ds.in_pkt);
+
+    if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
+        ff_thread_flush(avctx);
+    else if (avctx->codec->flush)
+        avctx->codec->flush(avctx);
+
+    avctx->pts_correction_last_pts =
+    avctx->pts_correction_last_dts = INT64_MIN;
+
+    if (av_codec_is_decoder(avctx->codec)) {
+        DecodeFilterContext *s = &avctx->internal->filter;
+
+        for (int i = 0; i < s->nb_bsfs; i++)
+            av_bsf_flush(s->bsfs[i]);
+    }
+
+    if (!avctx->refcounted_frames)
+        av_frame_unref(avci->to_free);
+}
+
 void avsubtitle_free(AVSubtitle *sub)
 {
     int i;