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.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.
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 <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.
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
