[FFmpeg-devel] [PATCH 1/2] avcodec/hevcdec: Check early whether film grain is supported, fix race

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Applying film grain happens after ff_thread_finish_setup(),
> so the parameters synced in hevc_update_thread_context() must not
> be modified. But this is exactly what happens in case applying
> film grain fails. (The likely result is that in case of frame threading
> an uninitialized frame is output.)
> 
> Given that it is actually very easy to know in advance whether
> ff_h274_apply_film_grain() supports a given set of parameters,
> one can check for this before ff_thread_finish_setup()
> and avoid allocating an unused buffer lateron.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
>  libavcodec/h274.h    | 15 +++++++++++++++
>  libavcodec/hevcdec.c | 19 ++++++++++++-------
>  libavcodec/hevcdec.h |  2 ++
>  3 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/h274.h b/libavcodec/h274.h
> index 920f6991fb..27a07e4a91 100644
> --- a/libavcodec/h274.h
> +++ b/libavcodec/h274.h
> @@ -40,11 +40,26 @@ typedef struct H274FilmGrainDatabase {
>      int16_t slice_tmp[64][64];
>  } H274FilmGrainDatabase;
>  
> +/**
> + * Check whether ff_h274_apply_film_grain() supports the given parameter combination.
> + *
> + * @param model_id model_id from AVFilmGrainParams to be supplied
> + * @param pix_fmt  pixel format of the frames to be supplied
> + */
> +static inline int ff_h274_film_grain_params_supported(int model_id, enum AVPixelFormat pix_fmt)
> +{
> +    return model_id == 1 && pix_fmt == AV_PIX_FMT_YUV420P;
> +}
> +
>  // Synthesizes film grain on top of `in` and stores the result to `out`. `out`
>  // must already have been allocated and set to the same size and format as
>  // `in`.
>  //
>  // Returns a negative error code on error, such as invalid params.
> +// If ff_h274_film_grain_params_supported() indicated that the parameters
> +// are supported, no error will be returned if the arguments given to
> +// ff_h274_film_grain_params_supported() coincide with actual values
> +// from the frames and params.
>  int ff_h274_apply_film_grain(AVFrame *out, const AVFrame *in,
>                               H274FilmGrainDatabase *db,
>                               const AVFilmGrainParams *params);
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index 81b9c5e089..2be62ddfb2 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -2884,6 +2884,14 @@ static int hevc_frame_start(HEVCContext *s)
>          !(s->avctx->export_side_data & AV_CODEC_EXPORT_DATA_FILM_GRAIN) &&
>          !s->avctx->hwaccel;
>  
> +    if (s->ref->needs_fg &&
> +        !ff_h274_film_grain_params_supported(s->sei.common.film_grain_characteristics.model_id,
> +                                             s->ref->frame->format)) {
> +        av_log_once(s->avctx, AV_LOG_WARNING, AV_LOG_DEBUG, &s->film_grain_warning_shown,
> +                    "Unsupported film grain parameters. Ignoring film grain.\n");
> +        s->ref->needs_fg = 0;
> +    }
> +
>      if (s->ref->needs_fg) {
>          s->ref->frame_grain->format = s->ref->frame->format;
>          s->ref->frame_grain->width = s->ref->frame->width;
> @@ -2922,19 +2930,14 @@ static int hevc_frame_end(HEVCContext *s)
>  {
>      HEVCFrame *out = s->ref;
>      const AVFrameSideData *sd;
> -    int ret;
> +    av_unused int ret;
>  
>      if (out->needs_fg) {
>          sd = av_frame_get_side_data(out->frame, AV_FRAME_DATA_FILM_GRAIN_PARAMS);
>          av_assert0(out->frame_grain->buf[0] && sd);
>          ret = ff_h274_apply_film_grain(out->frame_grain, out->frame, &s->h274db,
>                                         (AVFilmGrainParams *) sd->data);
> -
> -        if (ret < 0) {
> -            av_log(s->avctx, AV_LOG_WARNING, "Failed synthesizing film "
> -                   "grain, ignoring: %s\n", av_err2str(ret));
> -            out->needs_fg = 0;
> -        }
> +        av_assert1(ret >= 0);
>      }
>  
>      return 0;
> @@ -3574,6 +3577,8 @@ static int hevc_update_thread_context(AVCodecContext *dst,
>      s->threads_number      = s0->threads_number;
>      s->threads_type        = s0->threads_type;
>  
> +    s->film_grain_warning_shown = s0->film_grain_warning_shown;
> +
>      if (s0->eos) {
>          s->seq_decode = (s->seq_decode + 1) & HEVC_SEQUENCE_COUNTER_MASK;
>          s->max_ra = INT_MAX;
> diff --git a/libavcodec/hevcdec.h b/libavcodec/hevcdec.h
> index 94609e4699..9b232df68c 100644
> --- a/libavcodec/hevcdec.h
> +++ b/libavcodec/hevcdec.h
> @@ -596,6 +596,8 @@ typedef struct HEVCContext {
>      int nal_length_size;    ///< Number of bytes used for nal length (1, 2 or 4)
>      int nuh_layer_id;
>  
> +    int film_grain_warning_shown;
> +
>      AVBufferRef *rpu_buf;       ///< 0 or 1 Dolby Vision RPUs.
>      DOVIContext dovi_ctx;       ///< Dolby Vision decoding context
>  } HEVCContext;

Will apply the HEVC patch tomorrow unless there are objections.

- Andreas