[FFmpeg-devel] [PATCH v2] lavc/vvc: Set both s->pix_fmt & c->pix_fmt in set_output_format

[Wang, Fei W]

On Mon, 2025-01-13 at 13:10 +0000, Frank Plowman wrote:
> The logic in export_frame_params relies on the assumption that
> s->pix_fmt and c->pix_fmt always correspond to one another (although
> they are not necessarily equal to one another in the case of
> hardware-accelerated decoders).  Consequently, whenever we set
> s->pix_fmt we must also set c->pix_fmt and vice-versa.
> 
> Signed-off-by: Frank Plowman <post at frankplowman.com>
> ---
> @Fei Wang: Could you please check this?  I believe this is the source
> of
> the issue and it fixes the issue for the software decoder but I am
> not
> sure about hardware.  In particular, is output->format in
> set_output_format already a (potentially) HW pixfmt? I have assumed
> this
> is not the case and so used get_format here as well as in
> export_frame_params.  If output->format is already a HW pixfmt
> though,

output->format is HW pixfmt too in HW path. And it doesn't make sence
to call get_format twice. I don't know your scenarios, if you really
need to use AVCodecContext.pix_fmt to detect frame format change in
native soft path, maybe can consider to change in export_frame_params()
like:

pix_fmt = c->hwaccel ? s->pix_fmt : c->pix_fmt;

if (pix_fmt != sps->pix_fmt || ...) {
    ...
}

And better to add some explaination.

Thanks
Fei

> will we need to implement an inverse get_format function or is there
> some easier way to do this?
> ---
>  libavcodec/vvc/dec.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/vvc/dec.c b/libavcodec/vvc/dec.c
> index 1cb168de7e..070a191dda 100644
> --- a/libavcodec/vvc/dec.c
> +++ b/libavcodec/vvc/dec.c
> @@ -769,13 +769,13 @@ static int slice_start(SliceContext *sc,
> VVCContext *s, VVCFrameContext *fc,
>      return 0;
>  }
>  
> -static enum AVPixelFormat get_format(AVCodecContext *avctx, const
> VVCSPS *sps)
> +static enum AVPixelFormat get_format(AVCodecContext *avctx, enum
> AVPixelFormat pix_fmt)
>  {
>  #define HWACCEL_MAX CONFIG_VVC_VAAPI_HWACCEL
>  
>      enum AVPixelFormat pix_fmts[HWACCEL_MAX + 2], *fmt = pix_fmts;
>  
> -    switch (sps->pix_fmt) {
> +    switch (pix_fmt) {
>      case AV_PIX_FMT_YUV420P:
>  #if CONFIG_VVC_VAAPI_HWACCEL
>          *fmt++ = AV_PIX_FMT_VAAPI;
> @@ -788,7 +788,7 @@ static enum AVPixelFormat
> get_format(AVCodecContext *avctx, const VVCSPS *sps)
>          break;
>      }
>  
> -    *fmt++ = sps->pix_fmt;
> +    *fmt++ = pix_fmt;
>      *fmt = AV_PIX_FMT_NONE;
>  
>      return ff_get_format(avctx, pix_fmts);
> @@ -805,7 +805,7 @@ static int export_frame_params(VVCContext *s,
> const VVCFrameContext *fc)
>      if (s->pix_fmt != sps->pix_fmt || c->coded_width != pps->width
> || c->coded_height != pps->height) {
>          c->coded_width  = pps->width;
>          c->coded_height = pps->height;
> -        ret = get_format(c, sps);
> +        ret = get_format(c, sps->pix_fmt);
>          if (ret < 0)
>              return ret;
>  
> @@ -961,7 +961,7 @@ fail:
>      return ret;
>  }
>  
> -static int set_output_format(const VVCContext *s, const AVFrame
> *output)
> +static int set_output_format(VVCContext *s, const AVFrame *output)
>  {
>      AVCodecContext *c = s->avctx;
>      int ret;
> @@ -970,7 +970,11 @@ static int set_output_format(const VVCContext
> *s, const AVFrame *output)
>          if ((ret = ff_set_dimensions(c, output->width, output-
> >height)) < 0)
>              return ret;
>      }
> -    c->pix_fmt = output->format;
> +    ret = get_format(c, output->format);
> +    if (ret < 0)
> +        return ret;
> +    c->pix_fmt = ret;
> +    s->pix_fmt = output->format;
>      return 0;
>  }
>