[Mesa-dev] [PATCH] st/mesa: EGLImageTarget error handling

[Nicolai Hähnle]

On 21.03.2017 17:51, Philipp Zabel wrote:
> Stop trying to specify texture or renderbuffer objects for unsupported
> EGL images. Generate the error codes specified in the OES_EGL_image
> extension.
>
> Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> ---
> EGLImageTargetTexture2D and EGLImageTargetRenderbuffer would call
> the pipe driver's create_surface callback without ever checking that
> the given EGL image is actually compatible with the chosen target
> texture or renderbuffer. This patch adds a call to the pipe driver's
> is_format_supported callback and generates an INVALID_OPERATION error
> for unsupported EGL images. If the EGL image handle does not describe
> a valid EGL image, an INVALID_VALUE error is generated.

This description might as well go into the commit message IMO.

Do you have test cases for these errors? If so, best to mention it in 
the commit message as well.


> ---
>  src/mesa/state_tracker/st_cb_eglimage.c | 53 +++++++++++++++++++++++++++++++--
>  src/mesa/state_tracker/st_manager.c     | 27 ++++++-----------
>  src/mesa/state_tracker/st_manager.h     |  4 ++-
>  3 files changed, 63 insertions(+), 21 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_eglimage.c b/src/mesa/state_tracker/st_cb_eglimage.c
> index c425154..6810e24 100644
> --- a/src/mesa/state_tracker/st_cb_eglimage.c
> +++ b/src/mesa/state_tracker/st_cb_eglimage.c
> @@ -25,6 +25,7 @@
>   *    Chia-I Wu <olv at lunarg.com>
>   */
>
> +#include "main/errors.h"
>  #include "main/texobj.h"
>  #include "main/teximage.h"
>  #include "util/u_inlines.h"
> @@ -74,10 +75,34 @@ st_egl_image_target_renderbuffer_storage(struct gl_context *ctx,
>  					 GLeglImageOES image_handle)
>  {
>     struct st_context *st = st_context(ctx);
> +   struct st_manager *smapi =
> +      (struct st_manager *) st->iface.st_context_private;
>     struct st_renderbuffer *strb = st_renderbuffer(rb);
> +   struct pipe_screen *screen = st->pipe->screen;
> +   struct st_egl_image stimg;
>     struct pipe_surface *ps;
>
> -   ps = st_manager_get_egl_image_surface(st, (void *) image_handle);
> +   if (!smapi || !smapi->get_egl_image) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "glEGLImageTargetRenderbufferStorage");
> +      return NULL;
> +   }

This error should be caught by the caller in mesa/main.


> +
> +   memset(&stimg, 0, sizeof(stimg));
> +   if (!smapi->get_egl_image(smapi, (void *) image_handle, &stimg)) {
> +      /* image_handle does not refer to a valid EGL image object */
> +      _mesa_error(ctx, GL_INVALID_VALUE, "glEGLImageTargetRenderbufferStorage");
> +      return NULL;
> +   }
> +
> +   if (!screen->is_format_supported(screen, stimg.format, PIPE_TEXTURE_2D,
> +                                    stimg.texture->nr_samples,
> +                                    PIPE_BIND_RENDER_TARGET)) {
> +      /* unable to specify a texture object using the specified EGL image */
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "glEGLImageTargetRenderbufferStorage");
> +      return NULL;
> +   }
> +
> +   ps = st_manager_get_egl_image_surface(st, &stimg);
>     if (ps) {
>        strb->Base.Width = ps->width;
>        strb->Base.Height = ps->height;
> @@ -160,9 +185,33 @@ st_egl_image_target_texture_2d(struct gl_context *ctx, GLenum target,
>  			       GLeglImageOES image_handle)
>  {
>     struct st_context *st = st_context(ctx);
> +   struct st_manager *smapi =
> +      (struct st_manager *) st->iface.st_context_private;
> +   struct pipe_screen *screen = st->pipe->screen;
> +   struct st_egl_image stimg;
>     struct pipe_surface *ps;
>
> -   ps = st_manager_get_egl_image_surface(st, (void *) image_handle);
> +   if (!smapi || !smapi->get_egl_image) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "glEGLImageTargetTexture2D");
> +      return NULL;
> +   }

Same as above: should be caught by the caller.


> +
> +   memset(&stimg, 0, sizeof(stimg));
> +   if (!smapi->get_egl_image(smapi, (void *) image_handle, &stimg)) {
> +      /* image_handle does not refer to a valid EGL image object */
> +      _mesa_error(ctx, GL_INVALID_VALUE, "glEGLImageTargetTexture2D");
> +      return NULL;
> +   }
> +
> +   if (!screen->is_format_supported(screen, stimg.format, PIPE_TEXTURE_2D,
> +                                    stimg.texture->nr_samples,
> +                                    PIPE_BIND_SAMPLER_VIEW)) {
> +      /* unable to specify a texture object using the specified EGL image */
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "glEGLImageTargetTexture2D");
> +      return NULL;
> +   }
> +
> +   ps = st_manager_get_egl_image_surface(st, &stimg);

Since you're pretty much duplicating code here, have you considered 
adding a helper function that does all the above? Actually, I think 
st_manager_get_egl_image_surface should then be removed in favor of the 
helper function in st_cb_eglimage.c.


>     if (ps) {
>        st_bind_surface(ctx, target, texObj, texImage, ps);
>        pipe_surface_reference(&ps, NULL);
> diff --git a/src/mesa/state_tracker/st_manager.c b/src/mesa/state_tracker/st_manager.c
> index dad408a..8f16da1 100644
> --- a/src/mesa/state_tracker/st_manager.c
> +++ b/src/mesa/state_tracker/st_manager.c
> @@ -862,27 +862,18 @@ st_manager_flush_frontbuffer(struct st_context *st)
>   * FIXME: I think this should operate on resources, not surfaces
>   */
>  struct pipe_surface *
> -st_manager_get_egl_image_surface(struct st_context *st, void *eglimg)
> +st_manager_get_egl_image_surface(struct st_context *st,
> +                                 struct st_egl_image *stimg)
>  {
> -   struct st_manager *smapi =
> -      (struct st_manager *) st->iface.st_context_private;
> -   struct st_egl_image stimg;
>     struct pipe_surface *ps, surf_tmpl;
>
> -   if (!smapi || !smapi->get_egl_image)
> -      return NULL;
> -
> -   memset(&stimg, 0, sizeof(stimg));
> -   if (!smapi->get_egl_image(smapi, eglimg, &stimg))
> -      return NULL;
> -
> -   u_surface_default_template(&surf_tmpl, stimg.texture);
> -   surf_tmpl.format = stimg.format;
> -   surf_tmpl.u.tex.level = stimg.level;
> -   surf_tmpl.u.tex.first_layer = stimg.layer;
> -   surf_tmpl.u.tex.last_layer = stimg.layer;
> -   ps = st->pipe->create_surface(st->pipe, stimg.texture, &surf_tmpl);
> -   pipe_resource_reference(&stimg.texture, NULL);
> +   u_surface_default_template(&surf_tmpl, stimg->texture);
> +   surf_tmpl.format = stimg->format;
> +   surf_tmpl.u.tex.level = stimg->level;
> +   surf_tmpl.u.tex.first_layer = stimg->layer;
> +   surf_tmpl.u.tex.last_layer = stimg->layer;
> +   ps = st->pipe->create_surface(st->pipe, stimg->texture, &surf_tmpl);
> +   pipe_resource_reference(&stimg->texture, NULL);

Maybe moot if this function is removed anyway, but: the dereference 
should be moved into the caller, so that referencing/dereferencing 
logically happens in the same place.

Cheers,
Nicolai


>
>     return ps;
>  }
> diff --git a/src/mesa/state_tracker/st_manager.h b/src/mesa/state_tracker/st_manager.h
> index bbb9b0f..8490c56 100644
> --- a/src/mesa/state_tracker/st_manager.h
> +++ b/src/mesa/state_tracker/st_manager.h
> @@ -33,9 +33,11 @@
>  #include "pipe/p_compiler.h"
>
>  struct st_context;
> +struct st_egl_image;
>
>  struct pipe_surface *
> -st_manager_get_egl_image_surface(struct st_context *st, void *eglimg);
> +st_manager_get_egl_image_surface(struct st_context *st,
> +                                 struct st_egl_image *stimg);
>
>  void
>  st_manager_flush_frontbuffer(struct st_context *st);
>