[Mesa-dev] [PATCH] st/mesa: EGLImageTarget error handling
Nicolai Hähnle
nhaehnle at gmail.com
Wed Mar 22 10:53:12 UTC 2017
Hi Philipp,
On 22.03.2017 11:00, Philipp Zabel wrote:
> 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.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.
>
> Ok. If this is not considered too verbose, I'll move it up next time.
Please do. I think if anything, the commit messages in Mesa tend to be
too light on detail (I've been guilty of that myself...).
>> 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 <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.
>
> 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.
That seems wrong then. I'm not too familiar with osmesa, but either it
implements EGL, in which case it should probably implement the
get_egl_image callback, or it doesn't, in which case the extension
should be disabled.
The corresponding fix should probably go into a separate patch, though.
>>> +
>>> + 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.
Yeah, I think moving the helper into st_cb_eglimage.c and adding the two
parameters is the way to go. Please drop the st_manager_ prefix while
you're at it.
Cheers,
Nicolai
>>> 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
>
More information about the mesa-dev
mailing list