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

[Eric Engestrom]

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.

> Ideally Android will promote SwapInterval from a window to a display
> decision. Or app instance at least.
> 
> Until then I think we can live with eglSwapInterval being a no-op in said case.
> 
> -Emil