[Mesa-dev] [PATCH] st/vdpau: check that the format/bindings are valid

[Christian König]

Am 17.01.2014 04:19, schrieb Ilia Mirkin:
> It's a bit unreasonable to rely on applications doing the queries and
> then obeying their results.

Apart from a minor style comment (see below) the patch looks good to me, 
but isn't that a bit superfluous? I mean if the format isn't supported 
resource_create should probably return NULL anyway.


>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>
> We're starting to see people complaining about flash with newer versions of
> nouveau on nv30 cards. These cards don't actually expose any hw caps via
> vdpau, but the API is still free to use the surfaces/etc. We were seeing some
> errors in the logs that appear to only be able to happen if someone has
> managed to create an illegal-type surface. (And nv30 is pretty limited in its
> supported renderable format list.)

Or is it just that you guys print en error message to stderr when the 
format isn't supported and you want to suppress that?

>
> This adds checks to make sure that the surfaces are valid for the screen in
> question. Hopefully I haven't misunderstood gallium or the vdpau state
> tracker... My quick test with mplayer (but without hw accel) seems to work.
>
>   src/gallium/state_trackers/vdpau/bitmap.c        |  5 +++--
>   src/gallium/state_trackers/vdpau/output.c        | 16 ++++++++++------
>   src/gallium/state_trackers/vdpau/vdpau_private.h |  8 ++++++++
>   3 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/src/gallium/state_trackers/vdpau/bitmap.c b/src/gallium/state_trackers/vdpau/bitmap.c
> index 469f3e8..24dbc42 100644
> --- a/src/gallium/state_trackers/vdpau/bitmap.c
> +++ b/src/gallium/state_trackers/vdpau/bitmap.c
> @@ -43,7 +43,7 @@ vlVdpBitmapSurfaceCreate(VdpDevice device,
>                            VdpBitmapSurface *surface)
>   {
>      struct pipe_context *pipe;
> -   struct pipe_resource res_tmpl, *res;
> +   struct pipe_resource res_tmpl, *res = NULL;
>      struct pipe_sampler_view sv_templ;
>   
>      vlVdpBitmapSurface *vlsurface = NULL;
> @@ -79,7 +79,8 @@ vlVdpBitmapSurfaceCreate(VdpDevice device,
>      res_tmpl.usage = frequently_accessed ? PIPE_USAGE_DYNAMIC : PIPE_USAGE_STATIC;
>   
>      pipe_mutex_lock(dev->mutex);
> -   res = pipe->screen->resource_create(pipe->screen, &res_tmpl);
> +   if (CheckSurfaceParams(pipe->screen, &res_tmpl))
> +      res = pipe->screen->resource_create(pipe->screen, &res_tmpl);

I would rather code it like this:

if (!CheckSurfaceParams(pipe->screen, &res_tmpl))
     goto error_not supported;

It's a bit more obvious when you don't know the code.

That off course implies that we have error handling coded like this, 
feel free to change it if it isn't.

Christian.

>      if (!res) {
>         pipe_mutex_unlock(dev->mutex);
>         FREE(dev);
> diff --git a/src/gallium/state_trackers/vdpau/output.c b/src/gallium/state_trackers/vdpau/output.c
> index e4e1433..76c4611 100644
> --- a/src/gallium/state_trackers/vdpau/output.c
> +++ b/src/gallium/state_trackers/vdpau/output.c
> @@ -48,7 +48,7 @@ vlVdpOutputSurfaceCreate(VdpDevice device,
>                            VdpOutputSurface  *surface)
>   {
>      struct pipe_context *pipe;
> -   struct pipe_resource res_tmpl, *res;
> +   struct pipe_resource res_tmpl, *res = NULL;
>      struct pipe_sampler_view sv_templ;
>      struct pipe_surface surf_templ;
>   
> @@ -83,7 +83,9 @@ vlVdpOutputSurfaceCreate(VdpDevice device,
>      res_tmpl.usage = PIPE_USAGE_STATIC;
>   
>      pipe_mutex_lock(dev->mutex);
> -   res = pipe->screen->resource_create(pipe->screen, &res_tmpl);
> +
> +   if (CheckSurfaceParams(pipe->screen, &res_tmpl))
> +      res = pipe->screen->resource_create(pipe->screen, &res_tmpl);
>      if (!res) {
>         pipe_mutex_unlock(dev->mutex);
>         FREE(dev);
> @@ -117,7 +119,7 @@ vlVdpOutputSurfaceCreate(VdpDevice device,
>         FREE(dev);
>         return VDP_STATUS_ERROR;
>      }
> -
> +
>      pipe_resource_reference(&res, NULL);
>   
>      vl_compositor_init_state(&vlsurface->cstate, pipe);
> @@ -278,7 +280,7 @@ vlVdpOutputSurfacePutBitsIndexed(VdpOutputSurface surface,
>      enum pipe_format index_format;
>      enum pipe_format colortbl_format;
>   
> -   struct pipe_resource *res, res_tmpl;
> +   struct pipe_resource *res = NULL, res_tmpl;
>      struct pipe_sampler_view sv_tmpl;
>      struct pipe_sampler_view *sv_idx = NULL, *sv_tbl = NULL;
>   
> @@ -326,7 +328,8 @@ vlVdpOutputSurfacePutBitsIndexed(VdpOutputSurface surface,
>      pipe_mutex_lock(vlsurface->device->mutex);
>      vlVdpResolveDelayedRendering(vlsurface->device, NULL, NULL);
>   
> -   res = context->screen->resource_create(context->screen, &res_tmpl);
> +   if (CheckSurfaceParams(context->screen, &res_tmpl))
> +      res = context->screen->resource_create(context->screen, &res_tmpl);
>      if (!res)
>         goto error_resource;
>   
> @@ -359,7 +362,8 @@ vlVdpOutputSurfacePutBitsIndexed(VdpOutputSurface surface,
>      res_tmpl.usage = PIPE_USAGE_STAGING;
>      res_tmpl.bind = PIPE_BIND_SAMPLER_VIEW;
>   
> -   res = context->screen->resource_create(context->screen, &res_tmpl);
> +   if (CheckSurfaceParams(context->screen, &res_tmpl))
> +      res = context->screen->resource_create(context->screen, &res_tmpl);
>      if (!res)
>         goto error_resource;
>   
> diff --git a/src/gallium/state_trackers/vdpau/vdpau_private.h b/src/gallium/state_trackers/vdpau/vdpau_private.h
> index 60196ac..6c2c019 100644
> --- a/src/gallium/state_trackers/vdpau/vdpau_private.h
> +++ b/src/gallium/state_trackers/vdpau/vdpau_private.h
> @@ -332,6 +332,14 @@ RectToPipeBox(const VdpRect *rect, struct pipe_resource *res)
>      return box;
>   }
>   
> +static inline bool
> +CheckSurfaceParams(struct pipe_screen *screen,
> +                   const struct pipe_resource *templ)
> +{
> +   return screen->is_format_supported(
> +         screen, templ->format, templ->target, templ->nr_samples, templ->bind);
> +}
> +
>   typedef struct
>   {
>      struct vl_screen *vscreen;