On 2016/8/31 6:48, Mark Thompson wrote:
> On 30/08/16 09:00, Jun Zhao wrote:
>> v3 : fix sharpless mapping issue
>> v2 : fix filter support flag check logic issue
>
> Hi,
>
> A general remark to start: vf_scale_vaapi is named to be a scaling filter
> (i.e. it replaces vf_scale/swscale for AV_PIX_FMT_VAAPI) - is this therefore
> really the right place to be adding other operations unrelated to scaling?
>
> Do use-cases for these operations actually make sense to add here rather than
> in a separate filter? (I'm not sure what the answer to this should be - I
> would definitely argue that the deinterlacer should be a separate filter, but
> these other operations are unclear.)
>
>
>> From 415b00c6157d8311cc18713e6347400895f7333c Mon Sep 17 00:00:00 2001
>> From: Jun Zhao <mypopy...@gmail.com>
>> Date: Tue, 30 Aug 2016 14:36:00 +0800
>> Subject: [PATCH v3] lavf : scale_vaapi : add denoise/sharpless support.
>>
>> add denoise/sharpless support, used scope [-1, 100] as the input
>> scope.
>>
>> Signed-off-by: Jun Zhao <jun.z...@intel.com>
>> ---
>> libavfilter/vf_scale_vaapi.c | 115
>> +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 115 insertions(+)
>>
>> diff --git a/libavfilter/vf_scale_vaapi.c b/libavfilter/vf_scale_vaapi.c
>> index 8dd5acf..5658746 100644
>> --- a/libavfilter/vf_scale_vaapi.c
>> +++ b/libavfilter/vf_scale_vaapi.c
>> @@ -53,6 +53,16 @@ typedef struct ScaleVAAPIContext {
>> int output_width;
>> int output_height;
>>
>> + VAProcFilterCap denoise_caps;
>> + int support_denoise;
>> + int denoise; // enable denoise algo. level is the optional
>> + // value from the interval [-1, 100], -1 means
>> disabled
>> +
>> + VAProcFilterCap sharpless_caps;
>> + int support_sharpless;
>> + int sharpless; // enable sharpless. level is the optional value
>> + // from the interval [-1, 100], -1 means disabled
>
> "sharpless"? "sharpness" or "sharpen", might be better. (The filter is
> called "sharpening", though maybe it can also blur the image?)
>
agree, sharpless is a typo for sharpness
>> +
>> } ScaleVAAPIContext;
>>
>>
>> @@ -117,6 +127,8 @@ static int scale_vaapi_config_output(AVFilterLink
>> *outlink)
>> AVVAAPIFramesContext *va_frames;
>> VAStatus vas;
>> int err, i;
>> + unsigned int num_denoise_caps = 1;
>> + unsigned int num_sharpless_caps = 1;
>>
>> scale_vaapi_pipeline_uninit(ctx);
>>
>> @@ -225,6 +237,29 @@ static int scale_vaapi_config_output(AVFilterLink
>> *outlink)
>> goto fail;
>> }
>>
>> + vas = vaQueryVideoProcFilterCaps(ctx->hwctx->display, ctx->va_context,
>> + VAProcFilterNoiseReduction,
>> + &ctx->denoise_caps, &num_denoise_caps);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(ctx, AV_LOG_WARNING, "Failed to query denoise caps "
>> + "context: %d (%s).\n", vas, vaErrorStr(vas));
>> + ctx->support_denoise = 0;
>> + } else {
>> + ctx->support_denoise = 1;
>> + }
>> +
>> + vas = vaQueryVideoProcFilterCaps(ctx->hwctx->display, ctx->va_context,
>> + VAProcFilterSharpening,
>> + &ctx->sharpless_caps,
>> &num_sharpless_caps);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(ctx, AV_LOG_WARNING, "Failed to query sharpless caps "
>> + "context: %d (%s).\n", vas, vaErrorStr(vas));
>> + ctx->support_sharpless = 0;
>> + } else {
>> + ctx->support_sharpless = 1;
>> + }
>
> If the user explicitly requests these filters that can still be ignored if
> they not supported? Maybe it would be better to just give up with an error
> message at that point.
>
> Similarly, the warning that they are not supported is unhelpful if the user
> didn't ask for them.
agree, I will change the logic when can't supported
>
>> +
>> +
>> av_freep(&hwconfig);
>> av_hwframe_constraints_free(&constraints);
>> return 0;
>> @@ -250,6 +285,14 @@ static int vaapi_proc_colour_standard(enum AVColorSpace
>> av_cs)
>> }
>> }
>>
>> +static float map_to_range(
>> + int input, int in_min, int in_max,
>> + float out_min, float out_max)
>> +{
>> + return (input - in_min) * (out_max - out_min) / (in_max - in_min) +
>> out_min;
>> +}
>> +
>> +
>> static int scale_vaapi_filter_frame(AVFilterLink *inlink, AVFrame
>> *input_frame)
>> {
>> AVFilterContext *avctx = inlink->dst;
>> @@ -259,6 +302,10 @@ static int scale_vaapi_filter_frame(AVFilterLink
>> *inlink, AVFrame *input_frame)
>> VASurfaceID input_surface, output_surface;
>> VAProcPipelineParameterBuffer params;
>> VABufferID params_id;
>> + VABufferID denoise_id;
>> + VABufferID sharpless_id;
>> + VABufferID filter_bufs[8];
>> + int num_filter_bufs = 0;
>> VAStatus vas;
>> int err;
>>
>> @@ -290,6 +337,43 @@ static int scale_vaapi_filter_frame(AVFilterLink
>> *inlink, AVFrame *input_frame)
>> av_log(ctx, AV_LOG_DEBUG, "Using surface %#x for scale output.\n",
>> output_surface);
>>
>> + if (ctx->denoise != -1 && ctx->support_denoise) {
>> + VAProcFilterParameterBuffer denoise;
>> + denoise.type = VAProcFilterNoiseReduction;
>> + denoise.value = map_to_range(ctx->denoise, 0, 100,
>> + ctx->denoise_caps.range.min_value,
>> + ctx->denoise_caps.range.max_value);
>> + vas = vaCreateBuffer(ctx->hwctx->display, ctx->va_context,
>> + VAProcFilterParameterBufferType, sizeof(denoise), 1,
>> + &denoise, &denoise_id);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(ctx, AV_LOG_ERROR, "Failed to create denoise parameter
>> buffer: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>> + err = AVERROR(EIO);
>> + goto fail;
>> + }
>> + filter_bufs[num_filter_bufs++] = denoise_id;
>> + }
>> +
>> + if (ctx->sharpless != -1 && ctx->support_sharpless) {
>> + VAProcFilterParameterBuffer sharpless;
>> + sharpless.type = VAProcFilterSharpening;
>> + sharpless.value = map_to_range(ctx->sharpless,
>> + 0, 100,
>> + ctx->sharpless_caps.range.min_value,
>> + ctx->sharpless_caps.range.max_value);
>> + vas = vaCreateBuffer(ctx->hwctx->display, ctx->va_context,
>> + VAProcFilterParameterBufferType, sizeof(sharpless),
>> 1,
>> + &sharpless, &sharpless_id);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(ctx, AV_LOG_ERROR, "Failed to create sharpless
>> parameter buffer: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>> + err = AVERROR(EIO);
>> + goto fail;
>> + }
>> + filter_bufs[num_filter_bufs++] = sharpless_id;
>> + }
>> +
>> memset(¶ms, 0, sizeof(params));
>>
>> params.surface = input_surface;
>> @@ -304,6 +388,11 @@ static int scale_vaapi_filter_frame(AVFilterLink
>> *inlink, AVFrame *input_frame)
>> params.pipeline_flags = 0;
>> params.filter_flags = VA_FILTER_SCALING_HQ;
>>
>> + if (num_filter_bufs) {
>> + params.filters = filter_bufs;
>> + params.num_filters = num_filter_bufs;
>> + }
>> +
>> vas = vaBeginPicture(ctx->hwctx->display,
>> ctx->va_context, output_surface);
>> if (vas != VA_STATUS_SUCCESS) {
>> @@ -351,6 +440,28 @@ static int scale_vaapi_filter_frame(AVFilterLink
>> *inlink, AVFrame *input_frame)
>> goto fail;
>> }
>>
>> + // This doesn't get freed automatically for some reason.
>> + if (ctx->denoise != -1 && ctx->support_denoise) {
>> + vas = vaDestroyBuffer(ctx->hwctx->display, denoise_id);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(ctx, AV_LOG_ERROR, "Failed to free denoise parameter
>> buffer: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>> + err = AVERROR(EIO);
>> + goto fail;
>> + }
>> + }
>> +
>> + // This doesn't get freed automatically for some reason.
>> + if (ctx->sharpless != -1 && ctx->support_sharpless) {
>> + vas = vaDestroyBuffer(ctx->hwctx->display, sharpless_id);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(ctx, AV_LOG_ERROR, "Failed to free sharpless parameter
>> buffer: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>> + err = AVERROR(EIO);
>> + goto fail;
>> + }
>> + }
>
> See
> <https://git.libav.org/?p=libav.git;a=commit;h=582d4211e00015b68626f77ce4af53161e2b1713>
> for some later context to the comment. Might we be better off without the
> repeated create/destroy here, given that vaRenderPicture() isn't going to
> free these subsidiary buffers in the general case?
>
I will double-check this commits 582d4211e00015b68626f77ce4af53161e2b1713
>> +
>> av_frame_copy_props(output_frame, input_frame);
>> av_frame_free(&input_frame);
>>
>
> The filter parameter buffers should be freed on failure (assuming they don't
> persist).
>
>> @@ -418,6 +529,10 @@ static const AVOption scale_vaapi_options[] = {
>> OFFSET(output_height), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX,
>> .flags = FLAGS },
>> { "format", "Output video format (software format of hardware frames)",
>> OFFSET(output_format_string), AV_OPT_TYPE_STRING, .flags = FLAGS },
>> + { "denoise", "denoise level [-1, 100], -1 means disabled",
>> + OFFSET(denoise), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 100, .flags =
>> FLAGS },
>> + { "sharpless", "sharpless level [-1, 100], -1 means disabled",
>> + OFFSET(sharpless), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 100, .flags =
>> FLAGS },
>> { NULL },
>> };
>>
>> --
>> 2.1.4
>>
>
> Finally, I tried to test this on a Skylake with recent git libva. I took a
> 1080p input video and ran:
>
> ./ffmpeg_g -y -threads 1 -vaapi_device /dev/dri/renderD128 -hwaccel vaapi
> -hwaccel_output_format vaapi -i in.mp4 -an -vf
> 'format=vaapi|nv12,hwupload,scale_vaapi=w=1280:h=720:sharpless=50' -c:v
> h264_vaapi -qp 20 out.mp4
>
> but it doesn't appear to be working as one might expect. I end up with a
> 720p output video consisting of the top left four-ninths of the input video,
> unscaled? It also has a very large slowdown: that command runs at ~80fps,
> but without the sharpness setting in it runs at ~300fps.
>
In my test bed, when add denosie/sharpness in VPP filter pipeline in transcode
case, performance drop about 10% ~ 15%
(130fps -> 110fps, IVY and Debian 8.2, used intel-driver/libva/ffmepg master
branch), I guess intel-deriver or libva have
some performance issue in different CPU. And I will double-check this in SKL.
> I also tried:
>
> ./ffmpeg_g -y -threads 1 -vaapi_device /dev/dri/renderD128 -hwaccel vaapi
> -hwaccel_output_format vaapi -i in.mp4 -an -vf
> 'format=vaapi|nv12,hwupload,scale_vaapi=w=1280:h=720:denoise=50' -c:v
> h264_vaapi -qp 20 out.mp4
>
> and got:
>
> Thread 1 "ffmpeg_g" received signal SIGSEGV, Segmentation fault.
> hsw_veb_pre_format_convert (ctx=ctx@entry=0x234b8e0,
> proc_ctx=proc_ctx@entry=0x23ab170) at ../../git/src/gen75_vpp_vebox.c:1382
> 1382 proc_ctx->width_input =
> proc_ctx->pipeline_param->surface_region->width;
> (gdb) bt
> #0 hsw_veb_pre_format_convert (ctx=ctx@entry=0x234b8e0,
> proc_ctx=proc_ctx@entry=0x23ab170) at ../../git/src/gen75_vpp_vebox.c:1382
> #1 0x00007ffff4438bc7 in gen9_vebox_process_picture
> (ctx=ctx@entry=0x234b8e0, proc_ctx=0x23ab170) at
> ../../git/src/gen75_vpp_vebox.c:2384
> #2 0x00007ffff443279b in gen75_vpp_vebox (ctx=ctx@entry=0x234b8e0,
> proc_ctx=proc_ctx@entry=0x23a75a0) at ../../git/src/gen75_picture_process.c:89
> #3 0x00007ffff4432e91 in gen75_proc_picture (ctx=0x234b8e0,
> profile=VAProfileNone, codec_state=<optimized out>, hw_context=0x23a75a0) at
> ../../git/src/gen75_picture_process.c:328
> #4 0x00007ffff705de5f in vaEndPicture (dpy=0x234b800, context=33554432) at
> ../../git/va/va.c:1232
> #5 0x0000000000593133 in scale_vaapi_filter_frame (inlink=0x23e8d20,
> input_frame=0x23aab20) at src/libavfilter/vf_scale_vaapi.c:426
> #6 0x0000000000455a6c in ff_filter_frame_framed (link=0x23e8d20,
> frame=0x23aab20) at src/libavfilter/avfilter.c:1134
> #7 0x0000000000455f62 in ff_filter_frame (link=0x23e8d20, frame=0x23aab20)
> at src/libavfilter/avfilter.c:1232
> #8 0x00000000005078d7 in hwupload_filter_frame (link=0x237cc00,
> input=0x23aab20) at src/libavfilter/vf_hwupload.c:162
> #9 0x0000000000455a6c in ff_filter_frame_framed (link=0x237cc00,
> frame=0x23aab20) at src/libavfilter/avfilter.c:1134
> #10 0x0000000000455f62 in ff_filter_frame (link=0x237cc00, frame=0x23aab20)
> at src/libavfilter/avfilter.c:1232
> #11 0x00000000004555a4 in default_filter_frame (link=0x237b180,
> frame=0x23aab20) at src/libavfilter/avfilter.c:1046
> #12 0x0000000000455a6c in ff_filter_frame_framed (link=0x237b180,
> frame=0x23aab20) at src/libavfilter/avfilter.c:1134
> #13 0x0000000000455f62 in ff_filter_frame (link=0x237b180, frame=0x23aab20)
> at src/libavfilter/avfilter.c:1232
> #14 0x000000000045bde8 in request_frame (link=0x237b180) at
> src/libavfilter/buffersrc.c:450
> #15 0x000000000045b675 in av_buffersrc_add_frame_internal (ctx=0x237b080,
> frame=0x23ba000, flags=4) at src/libavfilter/buffersrc.c:239
> #16 0x000000000045b344 in av_buffersrc_add_frame_flags (ctx=0x237b080,
> frame=0x23ba000, flags=4) at src/libavfilter/buffersrc.c:164
> #17 0x00000000004257b2 in decode_video (ist=0x244a2e0, pkt=0x7fffffffdc50,
> got_output=0x7fffffffdcac) at src/ffmpeg.c:2196
> #18 0x000000000042606e in process_input_packet (ist=0x244a2e0,
> pkt=0x7fffffffdda0, no_eof=0) at src/ffmpeg.c:2340
> #19 0x000000000042ca11 in process_input (file_index=0) at src/ffmpeg.c:4020
> #20 0x000000000042cd06 in transcode_step () at src/ffmpeg.c:4108
> #21 0x000000000042ce23 in transcode () at src/ffmpeg.c:4162
> #22 0x000000000042d474 in main (argc=20, argv=0x7fffffffe468) at
> src/ffmpeg.c:4357
> (gdb) p proc_ctx
> $1 = (struct intel_vebox_context *) 0x23ab170
> (gdb) p proc_ctx->pipeline_param
> $2 = (VAProcPipelineParameterBuffer *) 0x23ab100
> (gdb) p proc_ctx->pipeline_param->surface_region
> $3 = (const VARectangle *) 0x0
> (gdb) p proc_ctx->pipeline_param->surface_region->width
> Cannot access memory at address 0x4
>
>
> So, can you share how you are running this, and what the expected results are?
>
>
I think intel-driver commits
4f8d4b(https://cgit.freedesktop.org/vaapi/intel-driver/commit/?id=4f8d4b211b4f90ef26c356b8028c5435cd685952)
fix this crash. :)
> Thanks
>
> - Mark
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
