| Message ID | 1553794093-19942-1-git-send-email-shaofei.wang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
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@intel.com> > Reviewed-by: Jun, Zhao <jun.zhao@intel.com> > Reviewed-by: Haihao Xiang <haihao.xiang@intel.com> > --- > Previous code reviews: > 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 > > > 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);
> -----Original Message----- > From: Xiang, Haihao > Sent: Tuesday, May 28, 2019 12:23 PM > To: ffmpeg-devel@ffmpeg.org > Cc: Wang, Shaofei <shaofei.wang@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.wang@intel.com> > > Reviewed-by: Jun, Zhao <jun.zhao@intel.com> > > Reviewed-by: Haihao Xiang <haihao.xiang@intel.com> > > --- > > Previous code reviews: > > 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 > > > > > > 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);
On Tue, 2019-06-04 at 15:21 +0800, Wang, Shaofei wrote: > > -----Original Message----- > > From: Xiang, Haihao > > Sent: Tuesday, May 28, 2019 12:23 PM > > To: ffmpeg-devel@ffmpeg.org > > Cc: Wang, Shaofei <shaofei.wang@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 Is it reasonable when dimension is updated however the low level frame still use stale dimension info? Thanks Haihao > > > > 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@intel.com> > > > Reviewed-by: Jun, Zhao <jun.zhao@intel.com> > > > Reviewed-by: Haihao Xiang <haihao.xiang@intel.com> > > > --- > > > Previous code reviews: > > > 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 > > > > > > > > > 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);
> -----Original Message----- > From: Xiang, Haihao > Sent: Thursday, June 6, 2019 11:57 AM > To: ffmpeg-devel@ffmpeg.org; Wang, Shaofei <shaofei.wang@intel.com> > Subject: Re: [FFmpeg-devel] [PATCH v2] libavcodec/vp8dec: fix the > multi-thread HWAccel decode error > > On Tue, 2019-06-04 at 15:21 +0800, Wang, Shaofei wrote: > > > -----Original Message----- > > > From: Xiang, Haihao > > > Sent: Tuesday, May 28, 2019 12:23 PM > > > To: ffmpeg-devel@ffmpeg.org > > > Cc: Wang, Shaofei <shaofei.wang@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 > > Is it reasonable when dimension is updated however the low level frame still > use stale dimension info? > > Thanks > Haihao The low level frame, especially hw frame will just need to be updated once. For example, the init phase of the first thread will call update_dimension and init hwaccel by get_pixel_format, but not needed to update low level frame for the follow-up threads.
On Thu, 2019-06-06 at 15:21 +0800, Wang, Shaofei wrote: > > -----Original Message----- > > From: Xiang, Haihao > > Sent: Thursday, June 6, 2019 11:57 AM > > To: ffmpeg-devel@ffmpeg.org; Wang, Shaofei <shaofei.wang@intel.com> > > Subject: Re: [FFmpeg-devel] [PATCH v2] libavcodec/vp8dec: fix the > > multi-thread HWAccel decode error > > > > On Tue, 2019-06-04 at 15:21 +0800, Wang, Shaofei wrote: > > > > -----Original Message----- > > > > From: Xiang, Haihao > > > > Sent: Tuesday, May 28, 2019 12:23 PM > > > > To: ffmpeg-devel@ffmpeg.org > > > > Cc: Wang, Shaofei <shaofei.wang@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 > > > > Is it reasonable when dimension is updated however the low level frame still > > use stale dimension info? > > > > Thanks > > Haihao > > The low level frame, especially hw frame will just need to be updated once. > For example, the init phase of the first thread will call update_dimension and > init hwaccel by get_pixel_format, but not needed to update low level frame for > the follow-up threads. I get your point, thanks for detailed explanation.
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);