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

Submitted by Jun Zhao on Aug. 9, 2018, 7:09 a.m.

Details

Message ID 1533798542-2494-1-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao Aug. 9, 2018, 7:09 a.m.
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@intel.com>
Reviewed-by: Jun, Zhao <jun.zhao@intel.com>
---
 libavcodec/vp8.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

Comments

Haihao Xiang Sept. 21, 2018, 4:07 a.m.
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.

> 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?

> 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:

   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@intel.com>

> Reviewed-by: Jun, Zhao <jun.zhao@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);
mypopy@gmail.com Sept. 21, 2018, 8:05 a.m.
On Fri, Sep 21, 2018 at 12:08 PM Xiang, Haihao <haihao.xiang@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
>
>    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@intel.com>
> > Reviewed-by: Jun, Zhao <jun.zhao@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);
Haihao Xiang Sept. 25, 2018, 4:14 a.m.
On Fri, 2018-09-21 at 16:05 +0800, mypopy@gmail.com wrote:
> On Fri, Sep 21, 2018 at 12:08 PM Xiang, Haihao <haihao.xiang@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@intel.com>

> > > Reviewed-by: Jun, Zhao <jun.zhao@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@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Patch hide | download patch | download mbox

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);