| Message ID | GV1P250MB0737EE6DEA209D1B92E33C3F8FF4A@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/hevcdec: Check early whether film grain is supported, fix race | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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@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
Series LGTM.
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;
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@outlook.com> --- libavcodec/h274.h | 15 +++++++++++++++ libavcodec/hevcdec.c | 19 ++++++++++++------- libavcodec/hevcdec.h | 2 ++ 3 files changed, 29 insertions(+), 7 deletions(-)