| Message ID | 20171012082045.21146-2-zhong.li@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Oct 12, 2017 at 04:20:45PM +0800, Zhong Li wrote: > some reference pictures are not existed but valiad surface_ids are set, > (for example, the second field of the first key frame can be > P_Picture_Type but there is no preivous frame.) > then may cause some driver problems (e.g, segment fault on mesa/AMD driver). > > Signed-off-by: Zhong Li <zhong.li@intel.com> > --- [...] > diff --git a/libavcodec/vdpau_mpeg12.c b/libavcodec/vdpau_mpeg12.c > index b657007ee7..6d73d45f1a 100644 > --- a/libavcodec/vdpau_mpeg12.c > +++ b/libavcodec/vdpau_mpeg12.c > @@ -45,13 +45,18 @@ static int vdpau_mpeg_start_frame(AVCodecContext *avctx, > > switch (s->pict_type) { > case AV_PICTURE_TYPE_B: > - ref = ff_vdpau_get_surface_id(s->next_picture.f); > - assert(ref != VDP_INVALID_HANDLE); > - info->backward_reference = ref; > + if (s->next_picture_ptr) { > + ref = ff_vdpau_get_surface_id(s->next_picture.f); > + assert(ref != VDP_INVALID_HANDLE); > + info->backward_reference = ref; > + } > /* fall through to forward prediction */ > case AV_PICTURE_TYPE_P: > - ref = ff_vdpau_get_surface_id(s->last_picture.f); > - info->forward_reference = ref; > + if (s->last_picture_ptr) { > + ref = ff_vdpau_get_surface_id(s->last_picture.f); > + assert(ref != VDP_INVALID_HANDLE); please use av_assert*() [...]
2017-10-12 10:20 GMT+02:00 Zhong Li <zhong.li@intel.com>: > some reference pictures are not existed but valiad surface_ids are set, > (for example, the second field of the first key frame can be > P_Picture_Type but there is no preivous frame.) > then may cause some driver problems > (e.g, segment fault on mesa/AMD driver). Please also confirm that these driver bugs are fixed. Carl Eugen
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Michael Niedermayer > Sent: Friday, October 13, 2017 4:21 AM > To: FFmpeg development discussions and patches > <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V3 2/2] hwaccel_mpeg: check reference > pictures are available > > On Thu, Oct 12, 2017 at 04:20:45PM +0800, Zhong Li wrote: > > some reference pictures are not existed but valiad surface_ids are > > set, (for example, the second field of the first key frame can be > > P_Picture_Type but there is no preivous frame.) then may cause some > > driver problems (e.g, segment fault on mesa/AMD driver). > > > > Signed-off-by: Zhong Li <zhong.li@intel.com> > > --- > [...] > > > diff --git a/libavcodec/vdpau_mpeg12.c b/libavcodec/vdpau_mpeg12.c > > index b657007ee7..6d73d45f1a 100644 > > --- a/libavcodec/vdpau_mpeg12.c > > +++ b/libavcodec/vdpau_mpeg12.c > > @@ -45,13 +45,18 @@ static int > vdpau_mpeg_start_frame(AVCodecContext > > *avctx, > > > > switch (s->pict_type) { > > case AV_PICTURE_TYPE_B: > > - ref = ff_vdpau_get_surface_id(s->next_picture.f); > > - assert(ref != VDP_INVALID_HANDLE); > > - info->backward_reference = ref; > > + if (s->next_picture_ptr) { > > + ref = ff_vdpau_get_surface_id(s->next_picture.f); > > + assert(ref != VDP_INVALID_HANDLE); > > + info->backward_reference = ref; > > + } > > /* fall through to forward prediction */ > > > case AV_PICTURE_TYPE_P: > > - ref = ff_vdpau_get_surface_id(s->last_picture.f); > > - info->forward_reference = ref; > > + if (s->last_picture_ptr) { > > + ref = ff_vdpau_get_surface_id(s->last_picture.f); > > > + assert(ref != VDP_INVALID_HANDLE); > > please use av_assert*() Ok, will update. > > [...] > -- > Michael GnuPG fingerprint: > 9FF2128B147EF6730BADF133611EC787040B0FAB > > When you are offended at any man's fault, turn to yourself and study your > own failings. Then you will forget your anger. -- Epictetus
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Carl Eugen Hoyos > Sent: Friday, October 13, 2017 4:34 AM > To: FFmpeg development discussions and patches > <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V3 2/2] hwaccel_mpeg: check reference > pictures are available > > 2017-10-12 10:20 GMT+02:00 Zhong Li <zhong.li@intel.com>: > > some reference pictures are not existed but valiad surface_ids are > > set, (for example, the second field of the first key frame can be > > P_Picture_Type but there is no preivous frame.) then may cause some > > driver problems > > > (e.g, segment fault on mesa/AMD driver). > > Please also confirm that these driver bugs are fixed. Sorry for late reply. Do you mean this patch should be verified on mesa/AMD environment, or another patch of Meda/AMD driver is needed? > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
2017-10-23 10:49 GMT+02:00 Li, Zhong <zhong.li@intel.com>: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Carl Eugen Hoyos >> Sent: Friday, October 13, 2017 4:34 AM >> To: FFmpeg development discussions and patches >> <ffmpeg-devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH V3 2/2] hwaccel_mpeg: check reference >> pictures are available >> >> 2017-10-12 10:20 GMT+02:00 Zhong Li <zhong.li@intel.com>: >> > some reference pictures are not existed but valiad surface_ids are >> > set, (for example, the second field of the first key frame can be >> > P_Picture_Type but there is no preivous frame.) then may cause >> > some driver problems >> >> > (e.g, segment fault on mesa/AMD driver). >> >> Please also confirm that these driver bugs are fixed. > > Sorry for late reply. Do you mean this patch should be verified > on mesa/AMD environment, or another patch of Meda/AMD > driver is needed? I believe it is a good idea to add a work-around for a Mesa bug into the FFmpeg code-base (not all users will immediately update their graphics driver) but only if the Mesa bug is either fixed or at least going to be fixed with some certainty. The driver should never segfault. Carl Eugen
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Carl Eugen Hoyos > Sent: Monday, October 23, 2017 4:53 PM > To: FFmpeg development discussions and patches > <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH V3 2/2] hwaccel_mpeg: check reference > pictures are available > > 2017-10-23 10:49 GMT+02:00 Li, Zhong <zhong.li@intel.com>: > >> -----Original Message----- > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > Behalf > >> Of Carl Eugen Hoyos > >> Sent: Friday, October 13, 2017 4:34 AM > >> To: FFmpeg development discussions and patches > >> <ffmpeg-devel@ffmpeg.org> > >> Subject: Re: [FFmpeg-devel] [PATCH V3 2/2] hwaccel_mpeg: check > >> reference pictures are available > >> > >> 2017-10-12 10:20 GMT+02:00 Zhong Li <zhong.li@intel.com>: > >> > some reference pictures are not existed but valiad surface_ids are > >> > set, (for example, the second field of the first key frame can be > >> > P_Picture_Type but there is no preivous frame.) then may cause some > >> > driver problems > >> > >> > (e.g, segment fault on mesa/AMD driver). > >> > >> Please also confirm that these driver bugs are fixed. > > > > Sorry for late reply. Do you mean this patch should be verified on > > mesa/AMD environment, or another patch of Meda/AMD driver is needed? > > I believe it is a good idea to add a work-around for a Mesa bug into the > FFmpeg code-base (not all users will immediately update their graphics driver) > but only if the Mesa bug is either fixed or at least going to be fixed with > some certainty. > > The driver should never segfault. Exactly, a better way should be "return with some error message". But I have no AMD GPU on hand, this issue is reported by Mark Thompson when https://patchwork.ffmpeg.org/patch/5538/ applied. Maybe Mark can give some comments. BTW, this patch is not just a work-around, it improves the robustness. Application should make sure the parameters passing to driver are really valid. > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavcodec/dxva2_mpeg2.c b/libavcodec/dxva2_mpeg2.c index b7c69378f0..9117a3d7b1 100644 --- a/libavcodec/dxva2_mpeg2.c +++ b/libavcodec/dxva2_mpeg2.c @@ -51,11 +51,11 @@ static void fill_picture_parameters(AVCodecContext *avctx, memset(pp, 0, sizeof(*pp)); pp->wDecodedPictureIndex = ff_dxva2_get_surface_index(avctx, ctx, current_picture->f); pp->wDeblockedPictureIndex = 0; - if (s->pict_type != AV_PICTURE_TYPE_I) + if (s->pict_type != AV_PICTURE_TYPE_I && s->last_picture_ptr) pp->wForwardRefPictureIndex = ff_dxva2_get_surface_index(avctx, ctx, s->last_picture.f); else pp->wForwardRefPictureIndex = 0xffff; - if (s->pict_type == AV_PICTURE_TYPE_B) + if (s->pict_type == AV_PICTURE_TYPE_B && s->next_picture_ptr) pp->wBackwardRefPictureIndex = ff_dxva2_get_surface_index(avctx, ctx, s->next_picture.f); else pp->wBackwardRefPictureIndex = 0xffff; diff --git a/libavcodec/vaapi_mpeg2.c b/libavcodec/vaapi_mpeg2.c index 0d197c9692..942711b39f 100644 --- a/libavcodec/vaapi_mpeg2.c +++ b/libavcodec/vaapi_mpeg2.c @@ -73,10 +73,12 @@ static int vaapi_mpeg2_start_frame(AVCodecContext *avctx, av_unused const uint8_ switch (s->pict_type) { case AV_PICTURE_TYPE_B: - pic_param.backward_reference_picture = ff_vaapi_get_surface_id(s->next_picture.f); + if (s->next_picture_ptr) + pic_param.backward_reference_picture = ff_vaapi_get_surface_id(s->next_picture.f); // fall-through case AV_PICTURE_TYPE_P: - pic_param.forward_reference_picture = ff_vaapi_get_surface_id(s->last_picture.f); + if (s->last_picture_ptr) + pic_param.forward_reference_picture = ff_vaapi_get_surface_id(s->last_picture.f); break; } diff --git a/libavcodec/vdpau_mpeg12.c b/libavcodec/vdpau_mpeg12.c index b657007ee7..6d73d45f1a 100644 --- a/libavcodec/vdpau_mpeg12.c +++ b/libavcodec/vdpau_mpeg12.c @@ -45,13 +45,18 @@ static int vdpau_mpeg_start_frame(AVCodecContext *avctx, switch (s->pict_type) { case AV_PICTURE_TYPE_B: - ref = ff_vdpau_get_surface_id(s->next_picture.f); - assert(ref != VDP_INVALID_HANDLE); - info->backward_reference = ref; + if (s->next_picture_ptr) { + ref = ff_vdpau_get_surface_id(s->next_picture.f); + assert(ref != VDP_INVALID_HANDLE); + info->backward_reference = ref; + } /* fall through to forward prediction */ case AV_PICTURE_TYPE_P: - ref = ff_vdpau_get_surface_id(s->last_picture.f); - info->forward_reference = ref; + if (s->last_picture_ptr) { + ref = ff_vdpau_get_surface_id(s->last_picture.f); + assert(ref != VDP_INVALID_HANDLE); + info->forward_reference = ref; + } } info->slice_count = 0;
some reference pictures are not existed but valiad surface_ids are set, (for example, the second field of the first key frame can be P_Picture_Type but there is no preivous frame.) then may cause some driver problems (e.g, segment fault on mesa/AMD driver). Signed-off-by: Zhong Li <zhong.li@intel.com> --- libavcodec/dxva2_mpeg2.c | 4 ++-- libavcodec/vaapi_mpeg2.c | 6 ++++-- libavcodec/vdpau_mpeg12.c | 15 ++++++++++----- 3 files changed, 16 insertions(+), 9 deletions(-)