Hi Nicolai,
thank you for the comments.
On Wed, 2017-03-22 at 07:58 +0100, Nicolai Hähnle wrote:
> 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.za...@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.
Ok. If this is not considered too verbose, I'll move it up next time.
> Do you have test cases for these errors? If so, best to mention it in
> the commit message as well.
My test case is a GStreamer waylandsink displaying NV12 dmabufs under
weston on etnaviv:
gst-launch-1.0 v4l2src
device=/dev/v4l/by-path/platform-vivid.0-video-index0 io-mode=dmabuf !
video/x-raw,format=NV12 ! waylandsink
weston will create NV12 EGL images from the dmabufs, and pass those to
EGLImageTargetTexture2DOES. Without the is_format_supported check, this
will currently run into an assert in etna_rs_gen_clear_surface, called
from etna_create_surface, because of the unsupported format.
> > ---
> > 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 <o...@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.
How should that be detected?
_mesa_EGLImageTargetRenderbufferStorageOES can't directly access
((struct st_manager *)st_context(ctx)->iface.st_context_private)->get_egl_image.
I suppose checking ctx->Extensions.OES_EGL_image won't suffice, as that
seems to be enabled unconditionally, but osmesa doesn't implement the
get_egl_image callback.
> > +
> > + 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.
I considered passing the usage flag and error text into
st_manager_get_egl_image_surface at first, but decided against it
because of the indirection. Also there were no calls to _mesa_error in
st_manager.c, but in there are in other st_cb_* files already.
I could, move st_manager_get_egl_image_surface into st_cb_eglimage.c and
turn it into the helper you suggest.
>
> > 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.
Ok.
regards
Philipp
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
