diff mbox

[FFmpeg-devel,V2] lavc/vp8: Fix HWAccel VP8 decoder can't support resolution change.

Message ID 27a56d91-6d6d-dd12-794c-28d7fda99107@gmail.com
State Superseded
Headers show

Commit Message

Jun Zhao Nov. 29, 2017, 11:53 p.m. UTC 
V2: fix the V1 lead to webp crash issue.
From b943c2814789288d09b4032fa6cdfbc3fd672a2b Mon Sep 17 00:00:00 2001
From: Jun Zhao <jun.zhao@intel.com>
Date: Wed, 29 Nov 2017 10:22:03 +0800
Subject: [PATCH V2] lavc/vp8: Fix HWAccel VP8 decoder can't support resolution
 change.

Use the following command to reproduce this issue:
make fate-vp8-size-change HWACCEL="vaapi -vaapi_device \
/dev/dri/renderD128 -hwaccel_output_format yuv420p"
SAMPLES=../fate-suite/.

At the same time, reconstruct the public logic as a function.

Signed-off-by: Yun Zhou <yunx.z.zhou@intel.com>
Signed-off-by: Jun Zhao <jun.zhao@intel.com>
---
 libavcodec/vp8.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

Comments

Jun Zhao Dec. 5, 2017, 3:03 a.m. UTC | #1 
ping ?

On 2017/11/30 7:53, Jun Zhao wrote:
> V2: fix the V1 lead to webp crash issue.
Zhanbang He Dec. 5, 2017, 9:11 a.m. UTC | #2 
what is V1 and V2?

On Tue, Dec 5, 2017 at 11:03 AM, Jun Zhao <mypopydev@gmail.com> wrote:

> ping ?
>
> On 2017/11/30 7:53, Jun Zhao wrote:
> > V2: fix the V1 lead to webp crash issue.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Jun Zhao Dec. 12, 2017, 5:57 a.m. UTC | #3 
Re-ping, any comments?

On 2017/12/5 11:03, Jun Zhao wrote:
> ping ?
>
> On 2017/11/30 7:53, Jun Zhao wrote:
>> V2: fix the V1 lead to webp crash issue.
Mark Thompson Dec. 14, 2017, 12:51 a.m. UTC | #4 
On 29/11/17 23:53, Jun Zhao wrote:
> V2: fix the V1 lead to webp crash issue.
> 
> From b943c2814789288d09b4032fa6cdfbc3fd672a2b Mon Sep 17 00:00:00 2001
> From: Jun Zhao <jun.zhao@intel.com>
> Date: Wed, 29 Nov 2017 10:22:03 +0800
> Subject: [PATCH V2] lavc/vp8: Fix HWAccel VP8 decoder can't support resolution
>  change.
> 
> Use the following command to reproduce this issue:
> make fate-vp8-size-change HWACCEL="vaapi -vaapi_device \
> /dev/dri/renderD128 -hwaccel_output_format yuv420p"
> SAMPLES=../fate-suite/.
> 
> At the same time, reconstruct the public logic as a function.
> 
> Signed-off-by: Yun Zhou <yunx.z.zhou@intel.com>
> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> ---
>  libavcodec/vp8.c | 46 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
> index 471c0bb89e..d5cb7be7b3 100644
> --- a/libavcodec/vp8.c
> +++ b/libavcodec/vp8.c
> @@ -167,6 +167,30 @@ static VP8Frame *vp8_find_free_buffer(VP8Context *s)
>      return frame;
>  }
>  
> +static enum AVPixelFormat get_pixel_format(VP8Context *s)
> +{
> +    enum AVPixelFormat fmt;
> +    enum AVPixelFormat pix_fmts[] = {
> +#if CONFIG_VP8_VAAPI_HWACCEL
> +        AV_PIX_FMT_VAAPI,
> +#endif
> +#if CONFIG_VP8_NVDEC_HWACCEL
> +        AV_PIX_FMT_CUDA,
> +#endif
> +        AV_PIX_FMT_YUV420P,
> +        AV_PIX_FMT_NONE,
> +    };
> +
> +    fmt = ff_get_format(s->avctx, pix_fmts);
> +    if (fmt < 0) {
> +        fmt = AV_PIX_FMT_NONE;

ff_get_format() already returns either a format in the list or AV_PIX_FMT_NONE.

> +        av_log(s->avctx, AV_LOG_ERROR,
> +               "Can not support the format. \n");

This error message is meaningless.

I don't think an error message is appropriate here, anyway - either the user explicitly chose to fail (and already knows it) or something went wrong in ff_get_format() (which already prints a more useful error there).

> +    }
> +
> +    return fmt;

So I think just "return ff_get_format(...);"?

> +}
> +
>  static av_always_inline
>  int update_dimensions(VP8Context *s, int width, int height, int is_vp7)
>  {
> @@ -182,6 +206,15 @@ int update_dimensions(VP8Context *s, int width, int height, int is_vp7)
>              return ret;
>      }
>  
> +    if (!s->actually_webp && !is_vp7) {
> +        s->pix_fmt = get_pixel_format(s);
> +        if (s->pix_fmt < 0) {
> +            ret = AVERROR(EINVAL);
> +            return ret;

Just "return AVERROR(EINVAL);"?

> +        }
> +        avctx->pix_fmt = s->pix_fmt;
> +    }
> +
>      s->mb_width  = (s->avctx->coded_width  + 15) / 16;
>      s->mb_height = (s->avctx->coded_height + 15) / 16;
>  
> @@ -2598,18 +2631,7 @@ int vp78_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
>      if (s->actually_webp) {
>          // avctx->pix_fmt already set in caller.
>      } else if (!is_vp7 && s->pix_fmt == AV_PIX_FMT_NONE) {
> -        enum AVPixelFormat pix_fmts[] = {
> -#if CONFIG_VP8_VAAPI_HWACCEL
> -            AV_PIX_FMT_VAAPI,
> -#endif
> -#if CONFIG_VP8_NVDEC_HWACCEL
> -            AV_PIX_FMT_CUDA,
> -#endif
> -            AV_PIX_FMT_YUV420P,
> -            AV_PIX_FMT_NONE,
> -        };
> -
> -        s->pix_fmt = ff_get_format(s->avctx, pix_fmts);
> +        s->pix_fmt = get_pixel_format(s);
>          if (s->pix_fmt < 0) {
>              ret = AVERROR(EINVAL);
>              goto err;
> -- 
> 2.14.1
> 

Tested with VAAPI, logic LGTM.

Thanks,

- Mark
Jun Zhao Dec. 14, 2017, 1:05 a.m. UTC | #5 
On 2017/12/14 8:51, Mark Thompson wrote:
> On 29/11/17 23:53, Jun Zhao wrote:
>> V2: fix the V1 lead to webp crash issue.
>>
>> From b943c2814789288d09b4032fa6cdfbc3fd672a2b Mon Sep 17 00:00:00 2001
>> From: Jun Zhao <jun.zhao@intel.com>
>> Date: Wed, 29 Nov 2017 10:22:03 +0800
>> Subject: [PATCH V2] lavc/vp8: Fix HWAccel VP8 decoder can't support resolution
>>  change.
>>
>> Use the following command to reproduce this issue:
>> make fate-vp8-size-change HWACCEL="vaapi -vaapi_device \
>> /dev/dri/renderD128 -hwaccel_output_format yuv420p"
>> SAMPLES=../fate-suite/.
>>
>> At the same time, reconstruct the public logic as a function.
>>
>> Signed-off-by: Yun Zhou <yunx.z.zhou@intel.com>
>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>> ---
>>  libavcodec/vp8.c | 46 ++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 34 insertions(+), 12 deletions(-)
>>
>> diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
>> index 471c0bb89e..d5cb7be7b3 100644
>> --- a/libavcodec/vp8.c
>> +++ b/libavcodec/vp8.c
>> @@ -167,6 +167,30 @@ static VP8Frame *vp8_find_free_buffer(VP8Context *s)
>>      return frame;
>>  }
>>  
>> +static enum AVPixelFormat get_pixel_format(VP8Context *s)
>> +{
>> +    enum AVPixelFormat fmt;
>> +    enum AVPixelFormat pix_fmts[] = {
>> +#if CONFIG_VP8_VAAPI_HWACCEL
>> +        AV_PIX_FMT_VAAPI,
>> +#endif
>> +#if CONFIG_VP8_NVDEC_HWACCEL
>> +        AV_PIX_FMT_CUDA,
>> +#endif
>> +        AV_PIX_FMT_YUV420P,
>> +        AV_PIX_FMT_NONE,
>> +    };
>> +
>> +    fmt = ff_get_format(s->avctx, pix_fmts);
>> +    if (fmt < 0) {
>> +        fmt = AV_PIX_FMT_NONE;
> ff_get_format() already returns either a format in the list or AV_PIX_FMT_NONE.
>
>> +        av_log(s->avctx, AV_LOG_ERROR,
>> +               "Can not support the format. \n");
> This error message is meaningless.
>
> I don't think an error message is appropriate here, anyway - either the user explicitly chose to fail (and already knows it) or something went wrong in ff_get_format() (which already prints a more useful error there).
>
>> +    }
>> +
>> +    return fmt;
> So I think just "return ff_get_format(...);"?
Will double-check this part, Tks.
>
>> +}
>> +
>>  static av_always_inline
>>  int update_dimensions(VP8Context *s, int width, int height, int is_vp7)
>>  {
>> @@ -182,6 +206,15 @@ int update_dimensions(VP8Context *s, int width, int height, int is_vp7)
>>              return ret;
>>      }
>>  
>> +    if (!s->actually_webp && !is_vp7) {
>> +        s->pix_fmt = get_pixel_format(s);
>> +        if (s->pix_fmt < 0) {
>> +            ret = AVERROR(EINVAL);
>> +            return ret;
> Just "return AVERROR(EINVAL);"?
Yes, this change more pithy
>
>> +        }
>> +        avctx->pix_fmt = s->pix_fmt;
>> +    }
>> +
>>      s->mb_width  = (s->avctx->coded_width  + 15) / 16;
>>      s->mb_height = (s->avctx->coded_height + 15) / 16;
>>  
>> @@ -2598,18 +2631,7 @@ int vp78_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
>>      if (s->actually_webp) {
>>          // avctx->pix_fmt already set in caller.
>>      } else if (!is_vp7 && s->pix_fmt == AV_PIX_FMT_NONE) {
>> -        enum AVPixelFormat pix_fmts[] = {
>> -#if CONFIG_VP8_VAAPI_HWACCEL
>> -            AV_PIX_FMT_VAAPI,
>> -#endif
>> -#if CONFIG_VP8_NVDEC_HWACCEL
>> -            AV_PIX_FMT_CUDA,
>> -#endif
>> -            AV_PIX_FMT_YUV420P,
>> -            AV_PIX_FMT_NONE,
>> -        };
>> -
>> -        s->pix_fmt = ff_get_format(s->avctx, pix_fmts);
>> +        s->pix_fmt = get_pixel_format(s);
>>          if (s->pix_fmt < 0) {
>>              ret = AVERROR(EINVAL);
>>              goto err;
>> -- 
>> 2.14.1
>>
> Tested with VAAPI, logic LGTM.
>
> Thanks,
Thanks for the reviewed and tested, will re-submit after clean the code.
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
index 471c0bb89e..d5cb7be7b3 100644
--- a/libavcodec/vp8.c
+++ b/libavcodec/vp8.c
@@ -167,6 +167,30 @@  static VP8Frame *vp8_find_free_buffer(VP8Context *s)
     return frame;
 }
 
+static enum AVPixelFormat get_pixel_format(VP8Context *s)
+{
+    enum AVPixelFormat fmt;
+    enum AVPixelFormat pix_fmts[] = {
+#if CONFIG_VP8_VAAPI_HWACCEL
+        AV_PIX_FMT_VAAPI,
+#endif
+#if CONFIG_VP8_NVDEC_HWACCEL
+        AV_PIX_FMT_CUDA,
+#endif
+        AV_PIX_FMT_YUV420P,
+        AV_PIX_FMT_NONE,
+    };
+
+    fmt = ff_get_format(s->avctx, pix_fmts);
+    if (fmt < 0) {
+        fmt = AV_PIX_FMT_NONE;
+        av_log(s->avctx, AV_LOG_ERROR,
+               "Can not support the format. \n");
+    }
+
+    return fmt;
+}
+
 static av_always_inline
 int update_dimensions(VP8Context *s, int width, int height, int is_vp7)
 {
@@ -182,6 +206,15 @@  int update_dimensions(VP8Context *s, int width, int height, int is_vp7)
             return ret;
     }
 
+    if (!s->actually_webp && !is_vp7) {
+        s->pix_fmt = get_pixel_format(s);
+        if (s->pix_fmt < 0) {
+            ret = AVERROR(EINVAL);
+            return ret;
+        }
+        avctx->pix_fmt = s->pix_fmt;
+    }
+
     s->mb_width  = (s->avctx->coded_width  + 15) / 16;
     s->mb_height = (s->avctx->coded_height + 15) / 16;
 
@@ -2598,18 +2631,7 @@  int vp78_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
     if (s->actually_webp) {
         // avctx->pix_fmt already set in caller.
     } else if (!is_vp7 && s->pix_fmt == AV_PIX_FMT_NONE) {
-        enum AVPixelFormat pix_fmts[] = {
-#if CONFIG_VP8_VAAPI_HWACCEL
-            AV_PIX_FMT_VAAPI,
-#endif
-#if CONFIG_VP8_NVDEC_HWACCEL
-            AV_PIX_FMT_CUDA,
-#endif
-            AV_PIX_FMT_YUV420P,
-            AV_PIX_FMT_NONE,
-        };
-
-        s->pix_fmt = ff_get_format(s->avctx, pix_fmts);
+        s->pix_fmt = get_pixel_format(s);
         if (s->pix_fmt < 0) {
             ret = AVERROR(EINVAL);
             goto err;