| Message ID | 20200227180202.30982-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Encoding API code restructuration | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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.'
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?
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.
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.
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
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 --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;
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(-)