On 8 August 2017 at 10:00, Eric Engestrom <[email protected]> wrote: > On Tuesday, 2017-08-08 11:31:36 +0300, Tapani Pälli wrote: >> Currently swrastGetDrawableInfo always initializes w and h, patch >> refactors function as x11_get_drawable_info that returns success and >> sets the values only if no error happened. Add swrastGetDrawableInfo >> wrapper function as expected by DRI extension. >> >> Signed-off-by: Tapani Pälli <[email protected]> >> Reported-by: Emil Velikov <[email protected]> > > Couple notes below, but with that: > Reviewed-by: Eric Engestrom <[email protected]> > >> --- >> src/egl/drivers/dri2/platform_x11.c | 23 ++++++++++++++++------- >> 1 file changed, 16 insertions(+), 7 deletions(-) >> >> diff --git a/src/egl/drivers/dri2/platform_x11.c >> b/src/egl/drivers/dri2/platform_x11.c >> index 4ce819f..80f2477 100644 >> --- a/src/egl/drivers/dri2/platform_x11.c >> +++ b/src/egl/drivers/dri2/platform_x11.c >> @@ -99,8 +99,8 @@ swrastDestroyDrawable(struct dri2_egl_display * dri2_dpy, >> xcb_free_gc(dri2_dpy->conn, dri2_surf->swapgc); >> } >> >> -static void >> -swrastGetDrawableInfo(__DRIdrawable * draw, >> +static bool >> +x11_get_drawable_info(__DRIdrawable * draw, >> int *x, int *y, int *w, int *h, >> void *loaderPrivate) >> { >> @@ -110,12 +110,12 @@ swrastGetDrawableInfo(__DRIdrawable * draw, >> xcb_get_geometry_cookie_t cookie; >> xcb_get_geometry_reply_t *reply; >> xcb_generic_error_t *error; >> + bool ret = false; > > Don't initialise `ret` here... > >> >> - *x = *y = *w = *h = 0; >> cookie = xcb_get_geometry (dri2_dpy->conn, dri2_surf->drawable); >> reply = xcb_get_geometry_reply (dri2_dpy->conn, cookie, &error); >> if (reply == NULL) >> - return; >> + return false; >> >> if (error != NULL) { >> _eglLog(_EGL_WARNING, "error in xcb_get_geometry"); > > but set it here: > > + ret = false; > >> @@ -125,8 +125,18 @@ swrastGetDrawableInfo(__DRIdrawable * draw, >> *y = reply->y; >> *w = reply->width; >> *h = reply->height; >> + ret = true; >> } >> free(reply); >> + return ret; >> +} >> + >> +static void >> +swrastGetDrawableInfo(__DRIdrawable * draw, >> + int *x, int *y, int *w, int *h, >> + void *loaderPrivate) >> +{ > > + *x = *y = *w = *h = 0; > > All the callers (swrast & drisw) expect these to be set unconditionally, > so we should initialise them here. > Initialize preemptively or set when x11_get_drawable_info() fails - either one will do. With Eric's suggestions Reviewed-by: Emil Velikov <[email protected]>
> (Note that __glXDRIGetDrawableInfo() is the only impl to not honour that; > might need to be fixed.) > AFAICT the function returns false on error, we should be fine. Plus it's a legacy DRI1 codepath, so personally I will be a bit cautious about subtle changes in behaviour -Emil _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
