[FFmpeg-devel,4/9] lavc: add generic-decode-layer private data

Move AVCodecInternal.nb_draining_errors to it, should should not be
visible outside of decode.c.
---
 libavcodec/avcodec.c          |  4 +++-
 libavcodec/avcodec_internal.h |  2 ++
 libavcodec/decode.c           | 22 ++++++++++++++++++++--
 libavcodec/internal.h         | 10 +++++++---
 libavcodec/pthread_frame.c    |  3 ++-
 5 files changed, 34 insertions(+), 7 deletions(-)

Anton Khirnov:
> Move AVCodecInternal.nb_draining_errors to it, should should not be
> visible outside of decode.c.
> ---
>  libavcodec/avcodec.c          |  4 +++-
>  libavcodec/avcodec_internal.h |  2 ++
>  libavcodec/decode.c           | 22 ++++++++++++++++++++--
>  libavcodec/internal.h         | 10 +++++++---
>  libavcodec/pthread_frame.c    |  3 ++-
>  5 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index a5cb6035b6..ff251d2dae 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -150,7 +150,9 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
>      if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE)
>          return AVERROR(EINVAL);
>  
> -    avci = av_mallocz(sizeof(*avci));
> +    avci = av_codec_is_decoder(codec) ?
> +        ff_decode_internal_alloc()    :
> +        av_mallocz(sizeof(AVCodecInternal));
>      if (!avci) {
>          ret = AVERROR(ENOMEM);
>          goto end;
> diff --git a/libavcodec/avcodec_internal.h b/libavcodec/avcodec_internal.h
> index 6ffe575c3e..f52f91e07c 100644
> --- a/libavcodec/avcodec_internal.h
> +++ b/libavcodec/avcodec_internal.h
> @@ -53,4 +53,6 @@ int ff_decode_preinit(struct AVCodecContext *avctx);
>  void ff_decode_flush_buffers(struct AVCodecContext *avctx);
>  void ff_encode_flush_buffers(struct AVCodecContext *avctx);
>  
> +struct AVCodecInternal *ff_decode_internal_alloc(void);
> +
>  #endif // AVCODEC_AVCODEC_INTERNAL_H
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index b5e5b4a2db..c070148b58 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -50,6 +50,11 @@
>  #include "internal.h"
>  #include "thread.h"
>  
> +struct DecodeContext {
> +    /* to prevent infinite loop on errors when draining */
> +    int nb_draining_errors;
> +};
> +
>  static int apply_param_change(AVCodecContext *avctx, const AVPacket *avpkt)
>  {
>      int ret;
> @@ -439,7 +444,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              int nb_errors_max = 20 + (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME ?
>                                  avctx->thread_count : 1);
>  
> -            if (avci->nb_draining_errors++ >= nb_errors_max) {
> +            if (avci->d->nb_draining_errors++ >= nb_errors_max) {
>                  av_log(avctx, AV_LOG_ERROR, "Too many errors when draining, this is a bug. "
>                         "Stop draining and force EOF.\n");
>                  avci->draining_done = 1;
> @@ -1752,5 +1757,18 @@ void ff_decode_flush_buffers(AVCodecContext *avctx)
>  
>      av_bsf_flush(avci->bsf);
>  
> -    avci->nb_draining_errors = 0;
> +    avci->d->nb_draining_errors = 0;
> +}
> +
> +AVCodecInternal *ff_decode_internal_alloc(void)
> +{
> +    struct Dummy {
> +        AVCodecInternal   i;
> +        DecodeContext     d;
> +    } *dummy = av_mallocz(sizeof(*dummy));
> +    if (!dummy)
> +        return NULL;
> +    dummy->i.d = &dummy->d;
> +
> +    return &dummy->i;
>  }
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index dceae182c0..b672092ac4 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -49,7 +49,14 @@
>  #   define STRIDE_ALIGN 8
>  #endif
>  
> +typedef struct DecodeContext DecodeContext;
> +
>  typedef struct AVCodecInternal {
> +    /**
> +     * Generic decoding private data.
> +     */
> +    DecodeContext *d;

This approach has the downside of adding unnecessary indirecations; it
furthermore adds pointers to DecodeContext and EncodeContext to
AVCodecInternal, as if both could be valid at the same time.

The above could be overcome by using the typical approach to access
these extra fields, as is used for FFStream etc.

Furthermore, I do not really think that is worth it: The number of
fields affected by it are just so small. Has encoder code ever tried to
set nb_draining_errors?

> +
>      /**
>       * When using frame-threaded decoding, this field is set for the first
>       * worker thread (e.g. to decode extradata just once).
> @@ -148,9 +155,6 @@ typedef struct AVCodecInternal {
>      AVFrame *buffer_frame;
>      int draining_done;
>  
> -    /* to prevent infinite loop on errors when draining */
> -    int nb_draining_errors;
> -
>      /* used when avctx flag AV_CODEC_FLAG_DROPCHANGED is set */
>      int changed_frames_dropped;
>      int initial_format;
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 008f3da43b..bc305f561f 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -28,6 +28,7 @@
>  #include <stdint.h>
>  
>  #include "avcodec.h"
> +#include "avcodec_internal.h"
>  #include "codec_internal.h"
>  #include "decode.h"
>  #include "hwconfig.h"
> @@ -815,7 +816,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free,
>      p->parent = fctx;
>      p->avctx  = copy;
>  
> -    copy->internal = av_mallocz(sizeof(*copy->internal));
> +    copy->internal = ff_decode_internal_alloc();
>      if (!copy->internal)
>          return AVERROR(ENOMEM);
>      copy->internal->thread_ctx = p;

Quoting Andreas Rheinhardt (2023-06-24 21:34:58)
> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > index dceae182c0..b672092ac4 100644
> > --- a/libavcodec/internal.h
> > +++ b/libavcodec/internal.h
> > @@ -49,7 +49,14 @@
> >  #   define STRIDE_ALIGN 8
> >  #endif
> >  
> > +typedef struct DecodeContext DecodeContext;
> > +
> >  typedef struct AVCodecInternal {
> > +    /**
> > +     * Generic decoding private data.
> > +     */
> > +    DecodeContext *d;
> 
> This approach has the downside of adding unnecessary indirecations; it
> furthermore adds pointers to DecodeContext and EncodeContext to
> AVCodecInternal, as if both could be valid at the same time.
> 
> The above could be overcome by using the typical approach to access
> these extra fields, as is used for FFStream etc.
> 
> Furthermore, I do not really think that is worth it: The number of
> fields affected by it are just so small. Has encoder code ever tried to
> set nb_draining_errors?

True, for decoding there are currently very few fields. But there are
many more that are encoding-only - I am not handling more of them in
this set because some of them are not trivial and I did not want to make
this patchset overly large, as its point is fixing the FATE test in 9/9.

And I belive there is a strong case for hiding generic-layer private
state from individual decoders/encoders, as it is prone to being
misunderstood and misused (e.g. AVCodecInternal.draining is currently
used in ways it most likely should not be).

Since the overhead of having a private decode context in addition to a
private encode context is very small, I prefer adding it now, at least
for consistency. Otherwise I'd have to add a shared encode-decode
context, which would be about the same amount of code.