On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov <emil.l.veli...@gmail.com> wrote: > > On 5 July 2018 at 17:17, Eric Engestrom <eric.engest...@intel.com> wrote: > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: > >> On 5 July 2018 at 10:53, Eric Engestrom <eric.engest...@intel.com> wrote: > >> > On Monday, 2018-07-02 14:12:44 +0530, samiuddi wrote: > >> >> This fixes crash due to NULL window when swap interval is set > >> >> for pbuffer surface. > >> >> > >> >> Test: CtsDisplayTestCases pass > >> >> > >> >> Signed-off-by: samiuddi <sami.uddin.moham...@intel.com> > >> >> --- > >> >> > >> >> Kindly ignore this patch > >> >> https://lists.freedesktop.org/archives/mesa-dev/2018-July/199098.html > >> >> > >> >> src/egl/drivers/dri2/platform_android.c | 2 +- > >> >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> > >> >> diff --git a/src/egl/drivers/dri2/platform_android.c > >> >> b/src/egl/drivers/dri2/platform_android.c > >> >> index ca8708a..b5b960a 100644 > >> >> --- a/src/egl/drivers/dri2/platform_android.c > >> >> +++ b/src/egl/drivers/dri2/platform_android.c > >> >> @@ -485,7 +485,7 @@ droid_swap_interval(_EGLDriver *drv, _EGLDisplay > >> >> *dpy, > >> >> struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf); > >> >> struct ANativeWindow *window = dri2_surf->window; > >> >> > >> >> - if (window->setSwapInterval(window, interval)) > >> >> + if (window && window->setSwapInterval(window, interval)) > >> >> return EGL_FALSE; > >> > > >> > Shouldn't we return false if we couldn't set the swap interval? > >> > > >> > I think this should be: > >> > if (!window || window->setSwapInterval(window, interval)) > >> > return EGL_FALSE; > >> > > >> Changing the patch as above will lead to eglSwapInterval consistently > >> failing for pbuffer surfaces. > > > > I'm not that familiar with pbuffers, but does SwapInterval really make > > sense for them? I thought you couldn't swap a pbuffer. > > > > If so, failing an invalid op seems like the right thing to do. > > Otherwise, I guess sure, no-op returning success is fine. > > > Looking at eglSwapInterval manpage [1] (with my annotation): > "eglSwapInterval — specifies the minimum number of video frame periods > per buffer swap for the _window_ associated with the current context." > While the spec does not mention window/pbuffer/pixmap at all - it > extensively refers to eglSwapBuffers. > > Wrt eglSwapBuffers manpage [2] and spec are consistent: > > "If surface is a pixel buffer or a pixmap, eglSwapBuffers has no > effect, and no error is generated." > > Perhaps it's wrong to set eglSwapInterval for !window surfaces, but > its sibling (eglSwapBuffers) does not error out. > Hence I doubt many users make a distinction between window and pbuffer > surfaces for eglSwap*. > > Worth bringing to the WG meeting - it' planned for 1st August. >
As I pointed out when I proposed this variant here: https://patchwork.freedesktop.org/patch/219313/ "Also, I don't think EGL_FALSE is the right return-value, as it doesn't seem the EGL 1.5 spec defines any such error. Also, for instance dri2_swap_interval returns EGL_TRUE when there's no driver-function, which further backs the "silent failure" in this case IMO." So I think EGL_TRUE is the correct return-value for now. If the spec gets changed, we can of course update our implementation. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
