| Message ID | 1533798542-2494-1-git-send-email-mypopydev@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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);
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);
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
> > > 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. Agree dim-reset should be 1, and this comment should be addressed.
2018-08-09 9:09 GMT+02:00, Jun Zhao <mypopydev@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@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) { Why is the new variable dim_reset needed? Wouldn't the patch be simpler if you used s->macroblocks_base here? Carl Eugen
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of > Carl Eugen Hoyos > Sent: Wednesday, March 6, 2019 3:49 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@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@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@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) { > > 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 > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
2019-03-06 9:25 GMT+01:00, Wang, Shaofei <shaofei.wang@intel.com>: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of >> Carl Eugen Hoyos >> Sent: Wednesday, March 6, 2019 3:49 PM >> To: FFmpeg development discussions and patches <ffmpeg-devel@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@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@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) { >> >> 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
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);