[FFmpeg-devel] [PATCH v2] libavcodec/vp8dec: fix the multi-thread HWAccel decode error

[Xiang, Haihao]

On Thu, 2019-03-28 at 13:28 -0400, Shaofei Wang wrote:
> Fix the issue: https://github.com/intel/media-driver/issues/317
> 
> the root cause is update_dimensions will be called multple times
> when decoder thread number is not only 1, but update_dimensions
> call get_pixel_format in each decode thread will trigger the
> hwaccel_uninit/hwaccel_init more than once. But only one hwaccel
> should be shared with all decode threads.
> in current context,
> there are 3 situations in the update_dimensions():
> 1. First time calling. No matter single thread or multithread,
>    get_pixel_format() should be called after dimensions were
>    set;
> 2. Dimention changed at the runtime. Dimention need to be
>    updated when macroblocks_base is already allocated,
>    get_pixel_format() should be called to recreate new frames
>    according to updated dimention;

s/Dimention/dimension ?

BTW this version of patch doesn't address the concern provided when reviewing
the first version of patch. 

When (width != s->avctx->width || height != s->avctx->height) is true,
ff_set_dimensions() is called even if s->macroblocks_base is not allocated, so 
why set dim_reset to (s->macroblocks_base != NULL)? I think dim_reset should be
set to 1.

>     if (width  != s->avctx->width || ((width+15)/16 != s->mb_width ||
> (height+15)/16 != s->mb_height) && s->macroblocks_base ||
>          height != s->avctx->height) {
> @@ -196,9 +196,12 @@ int update_dimensions(VP8Context *s, int width, int
> height, int is_vp7)
>          ret = ff_set_dimensions(s->avctx, width, height);
>          if (ret < 0)
>              return ret;
> +
> +        dim_reset = (s->macroblocks_base != NULL);


> 3. Multithread first time calling. After decoder init, the
>    other threads will call update_dimensions() at first time
>    to allocate macroblocks_base and set dimensions.
>    But get_pixel_format() is shouldn't be called due to low
>    level frames and context are already created.
> 
> In this fix, we only call update_dimensions as need.
> 
> Signed-off-by: Wang, Shaofei <shaofei.wang at intel.com>
> Reviewed-by: Jun, Zhao <jun.zhao at intel.com>
> Reviewed-by: Haihao Xiang <haihao.xiang at intel.com>
> ---
> Previous code reviews:
> 2019-03-06 9:25 GMT+01:00, Wang, Shaofei <shaofei.wang at intel.com>:
> > > -----Original Message-----
> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf 
> > > Of Carl Eugen Hoyos
> > > Sent: Wednesday, March 6, 2019 3:49 PM
> > > To: FFmpeg development discussions and patches 
> > > <ffmpeg-devel at ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] libavcodec/vp8dec: fix the 
> > > multi-thread HWAccel decode error
> > > 
> > > 2018-08-09 9:09 GMT+02:00, Jun Zhao <mypopydev at gmail.com>:
> > > > the root cause is update_dimentions call get_pixel_format will 
> > > > trigger the hwaccel_uninit/hwaccel_init , in current context, there 
> > > > are 3 situations in the update_dimentions():
> > > > 1. First time calling. No matter single thread or multithread,
> > > >    get_pixel_format() should be called after dimentions were
> > > >    set;
> > > > 2. Dimention changed at the runtime. Dimention need to be
> > > >    updated when macroblocks_base is already allocated,
> > > >    get_pixel_format() should be called to recreate new frames
> > > >    according to updated dimention;
> > > > 3. Multithread first time calling. After decoder init, the
> > > >    other threads will call update_dimentions() at first time
> > > >    to allocate macroblocks_base and set dimentions.
> > > >    But get_pixel_format() is shouldn't be called due to low
> > > >    level frames and context are already created.
> > > > In this fix, we only call update_dimentions as need.
> > > > 
> > > > Signed-off-by: Wang, Shaofei <shaofei.wang at intel.com>
> > > > Reviewed-by: Jun, Zhao <jun.zhao at intel.com>
> > > > ---
> > > >  libavcodec/vp8.c |    7 +++++--
> > > >  1 files changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c index 
> > > > 3adfeac..18d1ada 100644
> > > > --- a/libavcodec/vp8.c
> > > > +++ b/libavcodec/vp8.c
> > > > @@ -187,7 +187,7 @@ static av_always_inline  int 
> > > > update_dimensions(VP8Context *s, int width, int height, int is_vp7)  {
> > > >      AVCodecContext *avctx = s->avctx;
> > > > -    int i, ret;
> > > > +    int i, ret, dim_reset = 0;
> > > > 
> > > >      if (width  != s->avctx->width || ((width+15)/16 != s->mb_width 
> > > > > > 
> > > > 
> > > > (height+15)/16 != s->mb_height) && s->macroblocks_base ||
> > > >          height != s->avctx->height) { @@ -196,9 +196,12 @@ int 
> > > > update_dimensions(VP8Context *s, int width, int height, int is_vp7)
> > > >          ret = ff_set_dimensions(s->avctx, width, height);
> > > >          if (ret < 0)
> > > >              return ret;
> > > > +
> > > > +        dim_reset = (s->macroblocks_base != NULL);
> > > >      }
> > > > 
> > > > -    if (!s->actually_webp && !is_vp7) {
> > > > +    if ((s->pix_fmt == AV_PIX_FMT_NONE || dim_reset) &&
> > > > +         !s->actually_webp && !is_vp7) {
> > > 
> > > Why is the new variable dim_reset needed?
> > > Wouldn't the patch be simpler if you used s->macroblocks_base here?
> > 
> > Since dim_reset was set in the "if" segment, it equal to (width != 
> > s->avctx->width || ((width+15)/16 != s->mb_width ||
> > (height+15)/16 != s->mb_height) || height != s->avctx->height) &&
> > s->macroblocks_base
> 
> Thank you!
> 
> Carl Eugen
> 
> 
>  libavcodec/vp8.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
> index ba79e5f..0a7f38b 100644
> --- a/libavcodec/vp8.c
> +++ b/libavcodec/vp8.c
> @@ -187,7 +187,7 @@ static av_always_inline
>  int update_dimensions(VP8Context *s, int width, int height, int is_vp7)
>  {
>      AVCodecContext *avctx = s->avctx;
> -    int i, ret;
> +    int i, ret, dim_reset = 0;
>  
>      if (width  != s->avctx->width || ((width+15)/16 != s->mb_width ||
> (height+15)/16 != s->mb_height) && s->macroblocks_base ||
>          height != s->avctx->height) {
> @@ -196,9 +196,12 @@ int update_dimensions(VP8Context *s, int width, int
> height, int is_vp7)
>          ret = ff_set_dimensions(s->avctx, width, height);
>          if (ret < 0)
>              return ret;
> +
> +        dim_reset = (s->macroblocks_base != NULL);
>      }
>  
> -    if (!s->actually_webp && !is_vp7) {
> +    if ((s->pix_fmt == AV_PIX_FMT_NONE || dim_reset) &&
> +         !s->actually_webp && !is_vp7) {
>          s->pix_fmt = get_pixel_format(s);
>          if (s->pix_fmt < 0)
>              return AVERROR(EINVAL);