[FFmpeg-devel] lavc/qsv: skip the packet if decoding failure.

Submitted by Ruiling Song on Jan. 25, 2018, 8:37 a.m.

Details

Message ID 1516869432-30961-1-git-send-email-ruiling.song@intel.com
State New
Headers show

Commit Message

Ruiling Song Jan. 25, 2018, 8:37 a.m.
From: "Ruiling, Song" <ruiling.song@intel.com>

MediaSDK may fail to decode some frame, just skip it.
Otherwise, it will keep decoding the failure packet repeatedly
without processing any packet afterwards.

v2:
switch to using av_packet_unref().

Signed-off-by: Ruiling Song <ruiling.song@intel.com>
---
 libavcodec/qsvdec_h2645.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jun Zhao Jan. 26, 2018, 5:02 a.m.
On 2018/1/25 16:37, Ruiling Song wrote:
> From: "Ruiling, Song" <ruiling.song@intel.com>
>
> MediaSDK may fail to decode some frame, just skip it.
> Otherwise, it will keep decoding the failure packet repeatedly
> without processing any packet afterwards.
>
> v2:
> switch to using av_packet_unref().
>
> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
> ---
>  libavcodec/qsvdec_h2645.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
> index 5e00673..d92a150 100644
> --- a/libavcodec/qsvdec_h2645.c
> +++ b/libavcodec/qsvdec_h2645.c
> @@ -153,8 +153,12 @@ static int qsv_decode_frame(AVCodecContext *avctx, void *data,
>          }
>  
>          ret = ff_qsv_process_data(avctx, &s->qsv, frame, got_frame, &s->buffer_pkt);
> -        if (ret < 0)
> +        if (ret < 0) {
> +            /* Drop buffer_pkt when failed to decode the packet. Otherwise,
> +               the decoder will keep decoding the failure packet. */
> +            av_packet_unref(&s->buffer_pkt);

I think add a warning message more better than only drop the packet on
the quiet. 

>              return ret;
> +        }
>  
>          s->buffer_pkt.size -= ret;
>          s->buffer_pkt.data += ret;
Jun Zhao Jan. 26, 2018, 5:41 a.m.
On 2018/1/25 16:37, Ruiling Song wrote:
> From: "Ruiling, Song" <ruiling.song@intel.com>
>
> MediaSDK may fail to decode some frame, just skip it.
> Otherwise, it will keep decoding the failure packet repeatedly
> without processing any packet afterwards.
>
> v2:
> switch to using av_packet_unref().
>
> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
> ---
>  libavcodec/qsvdec_h2645.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
> index 5e00673..d92a150 100644
> --- a/libavcodec/qsvdec_h2645.c
> +++ b/libavcodec/qsvdec_h2645.c
> @@ -153,8 +153,12 @@ static int qsv_decode_frame(AVCodecContext *avctx, void *data,
>          }
>  
>          ret = ff_qsv_process_data(avctx, &s->qsv, frame, got_frame, &s->buffer_pkt);
> -        if (ret < 0)
> +        if (ret < 0) {
> +            /* Drop buffer_pkt when failed to decode the packet. Otherwise,
> +               the decoder will keep decoding the failure packet. */
> +            av_packet_unref(&s->buffer_pkt);

I think add a warning message more better than only drop the packet on
the quiet. 

>              return ret;
> +        }
>  
>          s->buffer_pkt.size -= ret;
>          s->buffer_pkt.data += ret;
Ruiling Song Jan. 26, 2018, 7:33 a.m.
> -----Original Message-----

> From: Jun Zhao [mailto:mypopydev@gmail.com]

> Sent: Friday, January 26, 2018 1:02 PM

> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>;

> Song, Ruiling <ruiling.song@intel.com>

> Subject: Re: [FFmpeg-devel] [PATCH] lavc/qsv: skip the packet if decoding failure.

> 

> 

> 

> On 2018/1/25 16:37, Ruiling Song wrote:

> > From: "Ruiling, Song" <ruiling.song@intel.com>

> >

> > MediaSDK may fail to decode some frame, just skip it.

> > Otherwise, it will keep decoding the failure packet repeatedly

> > without processing any packet afterwards.

> >

> > v2:

> > switch to using av_packet_unref().

> >

> > Signed-off-by: Ruiling Song <ruiling.song@intel.com>

> > ---

> >  libavcodec/qsvdec_h2645.c | 6 +++++-

> >  1 file changed, 5 insertions(+), 1 deletion(-)

> >

> > diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c

> > index 5e00673..d92a150 100644

> > --- a/libavcodec/qsvdec_h2645.c

> > +++ b/libavcodec/qsvdec_h2645.c

> > @@ -153,8 +153,12 @@ static int qsv_decode_frame(AVCodecContext *avctx,

> void *data,

> >          }

> >

> >          ret = ff_qsv_process_data(avctx, &s->qsv, frame, got_frame, &s-

> >buffer_pkt);

> > -        if (ret < 0)

> > +        if (ret < 0) {

> > +            /* Drop buffer_pkt when failed to decode the packet. Otherwise,

> > +               the decoder will keep decoding the failure packet. */

> > +            av_packet_unref(&s->buffer_pkt);

> 

> I think add a warning message more better than only drop the packet on

> the quiet.

What do you think "packet decoding failed, skip it."?
Usually some error message has already been printed out before skipping the packet.
I am OK to make a new version to address it. And thanks for your feedback.

Ruiling
> >              return ret;

> > +        }

> >

> >          s->buffer_pkt.size -= ret;

> >          s->buffer_pkt.data += ret;
Mark Thompson Jan. 26, 2018, 11:24 a.m.
On 26/01/18 07:33, Song, Ruiling wrote:
> 
> 
>> -----Original Message-----
>> From: Jun Zhao [mailto:mypopydev@gmail.com]
>> Sent: Friday, January 26, 2018 1:02 PM
>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>;
>> Song, Ruiling <ruiling.song@intel.com>
>> Subject: Re: [FFmpeg-devel] [PATCH] lavc/qsv: skip the packet if decoding failure.
>>
>>
>>
>> On 2018/1/25 16:37, Ruiling Song wrote:
>>> From: "Ruiling, Song" <ruiling.song@intel.com>
>>>
>>> MediaSDK may fail to decode some frame, just skip it.
>>> Otherwise, it will keep decoding the failure packet repeatedly
>>> without processing any packet afterwards.
>>>
>>> v2:
>>> switch to using av_packet_unref().
>>>
>>> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
>>> ---
>>>  libavcodec/qsvdec_h2645.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
>>> index 5e00673..d92a150 100644
>>> --- a/libavcodec/qsvdec_h2645.c
>>> +++ b/libavcodec/qsvdec_h2645.c
>>> @@ -153,8 +153,12 @@ static int qsv_decode_frame(AVCodecContext *avctx,
>> void *data,
>>>          }
>>>
>>>          ret = ff_qsv_process_data(avctx, &s->qsv, frame, got_frame, &s-
>>> buffer_pkt);
>>> -        if (ret < 0)
>>> +        if (ret < 0) {
>>> +            /* Drop buffer_pkt when failed to decode the packet. Otherwise,
>>> +               the decoder will keep decoding the failure packet. */
>>> +            av_packet_unref(&s->buffer_pkt);
>>
>> I think add a warning message more better than only drop the packet on
>> the quiet.
> What do you think "packet decoding failed, skip it."?
> Usually some error message has already been printed out before skipping the packet.

Yeah.  I think the only ways we get here without having already logged the failure are either an internal bug (the setup is broken somehow) or out-of-memory.  Neither of those would be improved by a generic "decoding failed" message here.

Anyway, this was applied in libav and has already been applied here as a merge.

Thanks,

- Mark

Patch hide | download patch | download mbox

diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c
index 5e00673..d92a150 100644
--- a/libavcodec/qsvdec_h2645.c
+++ b/libavcodec/qsvdec_h2645.c
@@ -153,8 +153,12 @@  static int qsv_decode_frame(AVCodecContext *avctx, void *data,
         }
 
         ret = ff_qsv_process_data(avctx, &s->qsv, frame, got_frame, &s->buffer_pkt);
-        if (ret < 0)
+        if (ret < 0) {
+            /* Drop buffer_pkt when failed to decode the packet. Otherwise,
+               the decoder will keep decoding the failure packet. */
+            av_packet_unref(&s->buffer_pkt);
             return ret;
+        }
 
         s->buffer_pkt.size -= ret;
         s->buffer_pkt.data += ret;