Thanks for the review.
On Wed, Jun 22, 2016 at 1:17 PM, Christian König <deathsim...@vodafone.de>
wrote:
> Am 21.06.2016 um 21:21 schrieb Nayan Deshmukh:
>
>> use bicubic filtering as high quality scaling L1.
>>
>> v2: fix a typo and add a newline to code
>>
>> v3: -render the unscaled image on a temporary surface (Christian)
>> -apply noise reduction and sharpness filter on
>> unscaled surface
>> -render the final scaled surface using bicubic
>> interpolation
>>
>> v4: support high quality scaling
>>
>> Signed-off-by: Nayan Deshmukh <nayan26deshm...@gmail.com>
>> ---
>> src/gallium/state_trackers/vdpau/mixer.c | 109
>> ++++++++++++++++++++---
>> src/gallium/state_trackers/vdpau/query.c | 1 +
>> src/gallium/state_trackers/vdpau/vdpau_private.h | 6 ++
>> 3 files changed, 102 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/vdpau/mixer.c
>> b/src/gallium/state_trackers/vdpau/mixer.c
>> index 65c3ce2..2a67ac2 100644
>> --- a/src/gallium/state_trackers/vdpau/mixer.c
>> +++ b/src/gallium/state_trackers/vdpau/mixer.c
>> @@ -82,7 +82,6 @@ vlVdpVideoMixerCreate(VdpDevice device,
>> switch (features[i]) {
>> /* they are valid, but we doesn't support them */
>> case VDP_VIDEO_MIXER_FEATURE_DEINTERLACE_TEMPORAL_SPATIAL:
>> - case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L2:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L3:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L4:
>> @@ -110,6 +109,9 @@ vlVdpVideoMixerCreate(VdpDevice device,
>> vmixer->luma_key.supported = true;
>> break;
>> + case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> + vmixer->bicubic.supported = true;
>> + break;
>> default: goto no_params;
>> }
>> }
>> @@ -202,6 +204,11 @@ vlVdpVideoMixerDestroy(VdpVideoMixer mixer)
>> vl_matrix_filter_cleanup(vmixer->sharpness.filter);
>> FREE(vmixer->sharpness.filter);
>> }
>> +
>> + if (vmixer->bicubic.filter) {
>> + vl_bicubic_filter_cleanup(vmixer->bicubic.filter);
>> + FREE(vmixer->bicubic.filter);
>> + }
>> pipe_mutex_unlock(vmixer->device->mutex);
>> DeviceReference(&vmixer->device, NULL);
>> @@ -230,9 +237,11 @@ VdpStatus vlVdpVideoMixerRender(VdpVideoMixer
>> mixer,
>> VdpLayer const *layers)
>> {
>> enum vl_compositor_deinterlace deinterlace;
>> - struct u_rect rect, clip, *prect;
>> + struct u_rect rect, clip, *prect, *rect_temp, dirty_area, temp;
>> unsigned i, layer = 0;
>> struct pipe_video_buffer *video_buffer;
>> + struct pipe_sampler_view *sampler_view;
>> + struct pipe_surface *surface;
>> vlVdpVideoMixer *vmixer;
>> vlVdpSurface *surf;
>> @@ -324,8 +333,48 @@ VdpStatus vlVdpVideoMixerRender(VdpVideoMixer mixer,
>> rect.y1 = surf->templat.height;
>> prect = ▭
>> }
>> +
>> + if(vmixer->bicubic.filter){
>> + struct pipe_context *pipe;
>> + struct pipe_resource res_tmpl, *res;
>> + struct pipe_sampler_view sv_templ;
>> + struct pipe_surface surf_templ;
>> +
>> + pipe = vmixer->device->context;
>> + memset(&res_tmpl, 0, sizeof(res_tmpl));
>> +
>> + res_tmpl.target = PIPE_TEXTURE_2D;
>> + res_tmpl.format = surf->templat.chroma_format;
>>
>
> That is incorrect. The resource format isn't related in any way to the
> chroma format. Please use the format of the destination surface here.
>
> I should probably use res_tmpl = dst->sampler_view->texture->format; Right?
> + res_tmpl.width0 = surf->templat.width;
>> + res_tmpl.height0 = surf->templat.height;
>> + res_tmpl.depth0 = 1;
>> + res_tmpl.array_size = 1;
>> + res_tmpl.bind = PIPE_BIND_SAMPLER_VIEW | PIPE_BIND_RENDER_TARGET |
>> + PIPE_BIND_LINEAR | PIPE_BIND_SHARED;
>>
>
> No need for PIPE_BIND_LINEAR or PIPE_BIND_SHARED here, we are not going to
> share the temporary texture with anybody.
>
> + res_tmpl.usage = PIPE_USAGE_DEFAULT;
>> +
>> + res = pipe->screen->resource_create(pipe->screen, &res_tmpl);
>> +
>> + vlVdpDefaultSamplerViewTemplate(&sv_templ, res);
>> + sampler_view = pipe->create_sampler_view(pipe, res, &sv_templ);
>> +
>> + memset(&surf_templ, 0, sizeof(surf_templ));
>> + surf_templ.format = res->format;
>> + surface = pipe->create_surface(pipe, res, &surf_templ);
>> +
>> + vl_compositor_reset_dirty_area(&dirty_area);
>> + rect_temp = prect;
>>
>
> You need to free the resource with pipe_resource_reference(&res, NULL);
> here, otherwise you will create quite a memory leak.
>
> Same thing is true for the surface and the sampler view, but you need
> those and can only free them later on. Easiest way to do this is probably
> to grab an extra reference in the else case as well.
>
> I can also check in the end if (surface != dst->surface) and then free the
surface and sampler_view if it's true.
> + }
>> +
>> + else{
>>
>
> Coding style here should be "} else {" if I remember correctly.
>
> + surface = dst->surface;
>> + sampler_view = dst->sampler_view;
>> + rect_temp = RectToPipe(destination_video_rect, &temp);
>> + dirty_area = dst->dirty_area;
>> + }
>> +
>> vl_compositor_set_buffer_layer(&vmixer->cstate, compositor, layer,
>> video_buffer, prect, NULL, deinterlace);
>> - vl_compositor_set_layer_dst_area(&vmixer->cstate, layer++,
>> RectToPipe(destination_video_rect, &rect));
>> + vl_compositor_set_layer_dst_area(&vmixer->cstate, layer++, rect_temp);
>> for (i = 0; i < layer_count; ++i) {
>> vlVdpOutputSurface *src = vlGetDataHTAB(layers->source_surface);
>> @@ -343,22 +392,22 @@ VdpStatus vlVdpVideoMixerRender(VdpVideoMixer mixer,
>> ++layers;
>> }
>> - vl_compositor_set_dst_clip(&vmixer->cstate,
>> RectToPipe(destination_rect, &clip));
>>
>
> That probably needs to be moved into the true case of the if and the clip
> handled by the bicubic filter as well.
>
> Same is true for the destination video rectangle as well.
>
>
if the bicubic scaling is off then we need to set dst_area and dst_clip in
vl_compositor, but if bicubic scaling is on then
dst_area and dst_clip should be set in bicubic_filter.
Regards,
Nayan.
> Regards,
> Christian.
>
>
> - if (!vmixer->noise_reduction.filter && !vmixer->sharpness.filter)
>> + if (!vmixer->noise_reduction.filter && !vmixer->sharpness.filter &&
>> !vmixer->bicubic.filter)
>> vlVdpSave4DelayedRendering(vmixer->device, destination_surface,
>> &vmixer->cstate);
>> else {
>> - vl_compositor_render(&vmixer->cstate, compositor, dst->surface,
>> &dst->dirty_area, true);
>> + vl_compositor_render(&vmixer->cstate, compositor, surface,
>> &dirty_area, true);
>> - /* applying the noise reduction after scaling is actually not
>> very
>> - clever, but currently we should avoid to copy around the image
>> - data once more. */
>> if (vmixer->noise_reduction.filter)
>> vl_median_filter_render(vmixer->noise_reduction.filter,
>> - dst->sampler_view, dst->surface);
>> + sampler_view, surface);
>> if (vmixer->sharpness.filter)
>> vl_matrix_filter_render(vmixer->sharpness.filter,
>> - dst->sampler_view, dst->surface);
>> + sampler_view, surface);
>> +
>> + if (vmixer->bicubic.filter)
>> + vl_bicubic_filter_render(vmixer->bicubic.filter,
>> + sampler_view, dst->surface);
>> }
>> pipe_mutex_unlock(vmixer->device->mutex);
>> @@ -461,6 +510,28 @@
>> vlVdpVideoMixerUpdateSharpnessFilter(vlVdpVideoMixer *vmixer)
>> }
>> /**
>> + * Update the bicubic filter
>> + */
>> +static void
>> +vlVdpVideoMixerUpdateBicubicFilter(vlVdpVideoMixer *vmixer)
>> +{
>> + assert(vmixer);
>> +
>> + /* if present remove the old filter first */
>> + if (vmixer->bicubic.filter) {
>> + vl_bicubic_filter_cleanup(vmixer->bicubic.filter);
>> + FREE(vmixer->bicubic.filter);
>> + vmixer->bicubic.filter = NULL;
>> + }
>> + /* and create a new filter as needed */
>> + if (vmixer->bicubic.enabled) {
>> + vmixer->bicubic.filter = MALLOC(sizeof(struct vl_bicubic_filter));
>> + vl_bicubic_filter_init(vmixer->bicubic.filter,
>> vmixer->device->context,
>> + vmixer->video_width, vmixer->video_height);
>> + }
>> +}
>> +
>> +/**
>> * Retrieve whether features were requested at creation time.
>> */
>> VdpStatus
>> @@ -483,7 +554,6 @@ vlVdpVideoMixerGetFeatureSupport(VdpVideoMixer mixer,
>> switch (features[i]) {
>> /* they are valid, but we doesn't support them */
>> case VDP_VIDEO_MIXER_FEATURE_DEINTERLACE_TEMPORAL_SPATIAL:
>> - case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L2:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L3:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L4:
>> @@ -512,6 +582,10 @@ vlVdpVideoMixerGetFeatureSupport(VdpVideoMixer mixer,
>> feature_supports[i] = vmixer->luma_key.supported;
>> break;
>> + case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> + feature_supports[i] = vmixer->bicubic.supported;
>> + break;
>> +
>> default:
>> return VDP_STATUS_INVALID_VIDEO_MIXER_FEATURE;
>> }
>> @@ -544,7 +618,6 @@ vlVdpVideoMixerSetFeatureEnables(VdpVideoMixer mixer,
>> switch (features[i]) {
>> /* they are valid, but we doesn't support them */
>> case VDP_VIDEO_MIXER_FEATURE_DEINTERLACE_TEMPORAL_SPATIAL:
>> - case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L2:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L3:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L4:
>> @@ -578,6 +651,11 @@ vlVdpVideoMixerSetFeatureEnables(VdpVideoMixer mixer,
>> vmixer->luma_key.luma_min,
>> vmixer->luma_key.luma_max);
>> break;
>> + case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> + vmixer->bicubic.enabled = feature_enables[i];
>> + vlVdpVideoMixerUpdateBicubicFilter(vmixer);
>> + break;
>> +
>> default:
>> pipe_mutex_unlock(vmixer->device->mutex);
>> return VDP_STATUS_INVALID_VIDEO_MIXER_FEATURE;
>> @@ -612,7 +690,6 @@ vlVdpVideoMixerGetFeatureEnables(VdpVideoMixer mixer,
>> /* they are valid, but we doesn't support them */
>> case VDP_VIDEO_MIXER_FEATURE_DEINTERLACE_TEMPORAL:
>> case VDP_VIDEO_MIXER_FEATURE_DEINTERLACE_TEMPORAL_SPATIAL:
>> - case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L2:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L3:
>> case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L4:
>> @@ -636,6 +713,10 @@ vlVdpVideoMixerGetFeatureEnables(VdpVideoMixer mixer,
>> feature_enables[i] = vmixer->luma_key.enabled;
>> break;
>> + case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> + feature_enables[i] = vmixer->bicubic.enabled;
>> + break;
>> +
>> default:
>> return VDP_STATUS_INVALID_VIDEO_MIXER_FEATURE;
>> }
>> diff --git a/src/gallium/state_trackers/vdpau/query.c
>> b/src/gallium/state_trackers/vdpau/query.c
>> index c3decc0..e69c9b1 100644
>> --- a/src/gallium/state_trackers/vdpau/query.c
>> +++ b/src/gallium/state_trackers/vdpau/query.c
>> @@ -471,6 +471,7 @@ vlVdpVideoMixerQueryFeatureSupport(VdpDevice device,
>> VdpVideoMixerFeature featur
>> case VDP_VIDEO_MIXER_FEATURE_NOISE_REDUCTION:
>> case VDP_VIDEO_MIXER_FEATURE_DEINTERLACE_TEMPORAL:
>> case VDP_VIDEO_MIXER_FEATURE_LUMA_KEY:
>> + case VDP_VIDEO_MIXER_FEATURE_HIGH_QUALITY_SCALING_L1:
>> *is_supported = VDP_TRUE;
>> break;
>> default:
>> diff --git a/src/gallium/state_trackers/vdpau/vdpau_private.h
>> b/src/gallium/state_trackers/vdpau/vdpau_private.h
>> index 8673c6a..bcd4bb1 100644
>> --- a/src/gallium/state_trackers/vdpau/vdpau_private.h
>> +++ b/src/gallium/state_trackers/vdpau/vdpau_private.h
>> @@ -45,6 +45,7 @@
>> #include "os/os_thread.h"
>> #include "vl/vl_video_buffer.h"
>> +#include "vl/vl_bicubic_filter.h"
>> #include "vl/vl_compositor.h"
>> #include "vl/vl_csc.h"
>> #include "vl/vl_deint_filter.h"
>> @@ -373,6 +374,11 @@ typedef struct
>> } deint;
>> struct {
>> + bool supported, enabled;
>> + struct vl_bicubic_filter *filter;
>> + } bicubic;
>> +
>> + struct {
>> bool supported, enabled;
>> unsigned level;
>> struct vl_median_filter *filter;
>>
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
