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

[Xiang, Haihao]

On Fri, 2018-09-21 at 16:05 +0800, mypopy at gmail.com wrote:
> On Fri, Sep 21, 2018 at 12:08 PM Xiang, Haihao <haihao.xiang at intel.com> wrote:
> > 
> > On Thu, 2018-08-09 at 15:09 +0800, Jun Zhao wrote:
> > > the root cause is update_dimentions call get_pixel_format will
> > 
> > Typo? s/dimentions/dimensions. And I see the same typo a few times in this
> > log.
> > 
> 
> Ha, a good catch.
> 
> > > trigger the hwaccel_uninit/hwaccel_init , in current context,
> > 
> > What is the decode error? Could you document the error a little or a link to
> > the
> > issue?
> > 
> 
> In mutil-thread decoding case with media-driver, Vp8 HWaccel decoder
> will carsh in some case.
> 
> > > 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,
> > 
> > According to the code, ff_set_dimensions is called to overwrite previously
> > setup
> > dimensions when height is changed no matter
> > s->macroblocks_base is allocated or not. I think the HW frames and context
> > should be re-created as well for this case
> > 
> > But I am curious why height and width are treated differently in this
> > condition:
> > 
> >     if (width  != s->avctx->width || ((width+15)/16 != s->mb_width ||
> > (height+15)/16 != s->mb_height) && s->macroblocks_base ||
> >         height != s->avctx->height) {
> >        ...
> >        ff_set_dimensions(...);
> > }
> > 
> > The condition for updating dimensions is simple in VP9:
> 
> Now I didn't test VP9 decoder in this case

Yes, this patch is for VP8 decoder only. What I mean is that ff_set_dimensions()
is called too when {width, height} are changed without allocated
macroblocks_base. So dim_reset should be 1 instead of (s->macroblocks_base !=
NULL) in the patch.

> > 
> >    if (!(s->pix_fmt == s->gf_fmt && w == s->w && h == s->h)) {
> >        ff_set_dimensions(...);
> >        ...
> >    }
> > 
> > >    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) {
> > >          s->pix_fmt = get_pixel_format(s);
> > >          if (s->pix_fmt < 0)
> > >              return AVERROR(EINVAL);
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel