> -----Original Message-----
> From: Xiang, Haihao
> Sent: Tuesday, May 28, 2019 12:23 PM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Wang, Shaofei <shaofei.w...@intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH v2] libavcodec/vp8dec: fix the
> multi-thread HWAccel decode error
>
> 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 ?
OK, should be 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 s->macroblocks_base is available, it means macroblocks_base of the context
has been already allocated by one of threads, so it's a reset operation when
(width != s->avctx->width...
If s->macroblocks_base is null, it's not allocated yet, and
(width != s->avctx->width..., just dimension need to be updated but it's not a
dim reset operation. Since we only call get_pixel_format() in the first thread
or
in the reset operation
> > 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.w...@intel.com>
> > Reviewed-by: Jun, Zhao <jun.z...@intel.com>
> > Reviewed-by: Haihao Xiang <haihao.xi...@intel.com>
> > ---
> > Previous code reviews:
> > 2019-03-06 9:25 GMT+01:00, Wang, Shaofei <shaofei.w...@intel.com>:
> > > > -----Original Message-----
> > > > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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 <mypopy...@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.w...@intel.com>
> > > > > Reviewed-by: Jun, Zhao <jun.z...@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);
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
