[Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.

[Erik Faye-Lund]

On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> On 5 July 2018 at 17:17, Eric Engestrom <eric.engestrom at 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.engestrom at 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.mohammad at 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.