[FFmpeg-devel,V3,2/2] hwaccel_mpeg: check reference pictures are available

Submitted by Zhong Li on Oct. 12, 2017, 8:20 a.m.

Details

Message ID 20171012082045.21146-2-zhong.li@intel.com
State New
Headers show

Commit Message

Zhong Li Oct. 12, 2017, 8:20 a.m.
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(-)

Comments

Michael Niedermayer Oct. 12, 2017, 8:21 p.m.
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*()

[...]
Carl Eugen Hoyos Oct. 12, 2017, 8:34 p.m.
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
Zhong Li Oct. 13, 2017, 5:24 a.m.
> -----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
Zhong Li Oct. 23, 2017, 8:49 a.m.
> -----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
Carl Eugen Hoyos Oct. 23, 2017, 8:52 a.m.
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
Zhong Li Oct. 23, 2017, 9:13 a.m.
> -----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

Patch hide | download patch | download mbox

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;