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

[Jun Zhao]

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 at 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 at intel.com>
>> Signed-off-by: Jun Zhao <jun.zhao at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel