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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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;

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".