Re: [Mesa-dev] [PATCH v4] st/vdpau: use bicubic filter for scaling

[Nayan Deshmukh]

Thanks for the review.

On Wed, Jun 22, 2016 at 1:17 PM, Christian König 
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 
>> ---
>>   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(>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(_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, _tmpl);
>> +
>> +  vlVdpDefaultSamplerViewTemplate(_templ, res);
>> +  sampler_view = pipe->create_sampler_view(pipe, res, _templ);
>> +
>> +  memset(_templ, 0, sizeof(surf_templ));
>> +  surf_templ.format = res->format;
>> +  surface = pipe->create_surface(pipe, res, _templ);
>> +
>> +  vl_compositor_reset_dirty_area(_area);
>> +  rect_temp = prect;
>>
>
> You need to free the resource with pipe_resource_reference(, 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) 

Re: [Mesa-dev] [PATCH v4] st/vdpau: use bicubic filter for scaling

[Christian König]

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 
---
  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(>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(_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.



+  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, _tmpl);
+
+  vlVdpDefaultSamplerViewTemplate(_templ, res);
+  sampler_view = pipe->create_sampler_view(pipe, res, _templ);
+
+  memset(_templ, 0, sizeof(surf_templ));
+  surf_templ.format = res->format;
+  surface = pipe->create_surface(pipe, res, _templ);
+
+  vl_compositor_reset_dirty_area(_area);
+  rect_temp = prect;


You need to free the resource with pipe_resource_reference(, 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.



+   }
+
+   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, );
+   dirty_area = dst->dirty_area;
+   }
+
 vl_compositor_set_buffer_layer(>cstate, compositor, layer, 
video_buffer, prect, NULL, deinterlace);
-   vl_compositor_set_layer_dst_area(>cstate, layer++, 
RectToPipe(destination_video_rect, ));
+   vl_compositor_set_layer_dst_area(>cstate, layer++, rect_temp);
  
 for (i = 0; i <