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

[Emil Velikov]

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.

-Emil

[1] https://www.khronos.org/registry/EGL/sdk/docs/man/html/eglSwapInterval.xhtml
[2] https://www.khronos.org/registry/EGL/sdk/docs/man/html/eglSwapBuffers.xhtml