[FFmpeg-devel,1/2] lavu/hwcontext_vaapi: dump more decoding error details.

Submitted by Jun Zhao on May 23, 2018, 10:29 a.m.

Details

Message ID 1527071360-8938-2-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao May 23, 2018, 10:29 a.m.
dump more decoding error details when sync surface fail.

Signed-off-by: Jun Zhao <mypopydev@gmail.com>
---
 libavutil/hwcontext_vaapi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Mark Thompson May 24, 2018, 12:05 a.m.
On 23/05/18 11:29, Jun Zhao wrote:
> dump more decoding error details when sync surface fail.
> 
> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> ---
>  libavutil/hwcontext_vaapi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
> index e55bb8d..5bdb02f 100644
> --- a/libavutil/hwcontext_vaapi.c
> +++ b/libavutil/hwcontext_vaapi.c
> @@ -742,6 +742,23 @@ static int vaapi_map_frame(AVHWFramesContext *hwfc,
>          av_log(hwfc, AV_LOG_ERROR, "Failed to sync surface "
>                 "%#x: %d (%s).\n", surface_id, vas, vaErrorStr(vas));
>          err = AVERROR(EIO);
> +        /* query decode detail error */
> +        if (vas == VA_STATUS_ERROR_DECODING_ERROR) {
> +            VASurfaceDecodeMBErrors *dec_err = NULL;
> +            int i;
> +            vas = vaQuerySurfaceError(hwctx->display, surface_id, VA_STATUS_ERROR_DECODING_ERROR,
> +                                      (void **)&dec_err);
> +            if (VA_STATUS_SUCCESS == vas) {
> +                if (NULL != dec_err) {

Why this check?

> +                    for (i = 0; dec_err[i].status != -1; i++) {

How many items can be in this list?  We probably don't want to print more than one in the error log if this can happen on every frame in a broken stream.

> +                          av_log(hwfc, AV_LOG_ERROR, "Decoding deatils error, "

"Decoding error details:" maybe?

> +                                 "type: %d, start mb: %d, end md: %d, num mbs: %d.\n",
> +                                 dec_err[i].decode_error_type, dec_err[i].start_mb,
> +                                 dec_err[i].end_mb, dec_err[i].num_mb);
> +                    }
> +                }
> +            }
> +        }
>          goto fail;
>      }
>  
> 

What is the lifetime of the returned pointer here?  This can certainly be called on any thread asynchronously.

It's a bit nasty to have this in the generic code rather than the decoder: very few actual uses are going to call SyncSurface directly on a decoder output - rather they will pass it to some other component (another VAAPI one, or export).  Could we instead put this inside the decoder, enabled by some debug option?  (Some sort of "catch decoder errors earlier at a cost of (possibly greatly) decreased performance" flag.)

- Mark
mypopy@gmail.com May 24, 2018, 12:58 a.m.
2018-05-24 8:05 GMT+08:00 Mark Thompson <sw@jkqxz.net>:
> On 23/05/18 11:29, Jun Zhao wrote:
>> dump more decoding error details when sync surface fail.
>>
>> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
>> ---
>>  libavutil/hwcontext_vaapi.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
>> index e55bb8d..5bdb02f 100644
>> --- a/libavutil/hwcontext_vaapi.c
>> +++ b/libavutil/hwcontext_vaapi.c
>> @@ -742,6 +742,23 @@ static int vaapi_map_frame(AVHWFramesContext *hwfc,
>>          av_log(hwfc, AV_LOG_ERROR, "Failed to sync surface "
>>                 "%#x: %d (%s).\n", surface_id, vas, vaErrorStr(vas));
>>          err = AVERROR(EIO);
>> +        /* query decode detail error */
>> +        if (vas == VA_STATUS_ERROR_DECODING_ERROR) {
>> +            VASurfaceDecodeMBErrors *dec_err = NULL;
>> +            int i;
>> +            vas = vaQuerySurfaceError(hwctx->display, surface_id, VA_STATUS_ERROR_DECODING_ERROR,
>> +                                      (void **)&dec_err);
>> +            if (VA_STATUS_SUCCESS == vas) {
>> +                if (NULL != dec_err) {
>
> Why this check?
maybe give a assert0?
>
>> +                    for (i = 0; dec_err[i].status != -1; i++) {
>
> How many items can be in this list?  We probably don't want to print more than one in the error log if this can happen on every frame in a broken stream.
Oh, from the API  define, "array is terminated if "status==-1" is
detected", so I don't know
the error entry number from VA-API level (but I checked the iHD source code,
I think iHD always return 2 error entries in this case). In current
FFmpeg VA-API
 HWaccel decoding logic, I believe if vaSyncSurface fail, FFmpeg will return the
error and exit, so we don't have a change to print the error message
every frame.

>> +                          av_log(hwfc, AV_LOG_ERROR, "Decoding deatils error, "
>
> "Decoding error details:" maybe?
Ok, will change it
>
>> +                                 "type: %d, start mb: %d, end md: %d, num mbs: %d.\n",
>> +                                 dec_err[i].decode_error_type, dec_err[i].start_mb,
>> +                                 dec_err[i].end_mb, dec_err[i].num_mb);
>> +                    }
>> +                }
>> +            }
>> +        }
>>          goto fail;
>>      }
>>
>>
>
> What is the lifetime of the returned pointer here?  This can certainly be called on any thread asynchronously.
Yes, I have the same thought about this API, basically, I think
vaQuerySurfaceError()
need to return the pointer and ask the caller to free the error
entries like getaddressinfo()/freeaddrinfo(),
and I think need to submit a patch to VA-API/iHD driver for this part.
>
> It's a bit nasty to have this in the generic code rather than the decoder: very few actual uses are going to call SyncSurface directly on a decoder output - rather they will pass it to some other component (another VAAPI one, or export).  Could we instead put this inside the decoder, enabled by some debug option?  (Some sort of "catch decoder errors earlier at a cost of (possibly greatly) decreased performance" flag.)
I can give a case, when I debug the ticket #7200, I try to dump the
YUV but fail,
so I want to get more decoding error from iHD driver.

use the command like this: ./ffmpeg_g -hwaccel vaapi -vaapi_device
/dev/dri/renderD128 -hwaccel_flags allow_profile_mismatch -i test.ts
-f null /dev/null

I think give a decoder option in this case is a good suggestion, I
will check this way. Thanks.
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Patch hide | download patch | download mbox

diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
index e55bb8d..5bdb02f 100644
--- a/libavutil/hwcontext_vaapi.c
+++ b/libavutil/hwcontext_vaapi.c
@@ -742,6 +742,23 @@  static int vaapi_map_frame(AVHWFramesContext *hwfc,
         av_log(hwfc, AV_LOG_ERROR, "Failed to sync surface "
                "%#x: %d (%s).\n", surface_id, vas, vaErrorStr(vas));
         err = AVERROR(EIO);
+        /* query decode detail error */
+        if (vas == VA_STATUS_ERROR_DECODING_ERROR) {
+            VASurfaceDecodeMBErrors *dec_err = NULL;
+            int i;
+            vas = vaQuerySurfaceError(hwctx->display, surface_id, VA_STATUS_ERROR_DECODING_ERROR,
+                                      (void **)&dec_err);
+            if (VA_STATUS_SUCCESS == vas) {
+                if (NULL != dec_err) {
+                    for (i = 0; dec_err[i].status != -1; i++) {
+                          av_log(hwfc, AV_LOG_ERROR, "Decoding deatils error, "
+                                 "type: %d, start mb: %d, end md: %d, num mbs: %d.\n",
+                                 dec_err[i].decode_error_type, dec_err[i].start_mb,
+                                 dec_err[i].end_mb, dec_err[i].num_mb);
+                    }
+                }
+            }
+        }
         goto fail;
     }