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

[Tomasz Figa]

On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> On 30 August 2018 at 11:41, Eric Engestrom <eric.engestrom at intel.com> wrote:
> > On Thursday, 2018-08-30 17:55:14 +0900, Tomasz Figa wrote:
> >> Hi Erik, Emil, Eric,
> >>
> >> On Tue, Jul 10, 2018 at 12:41 AM Erik Faye-Lund <kusmabite at gmail.com> wrote:
> >> >
> >> > 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.
> >>
> >> What happens to this patch in the end? It looks like we're observing a
> >> similar problem in Chrome OS.
> >>
> >> Emil, was there any follow-up on the WG meeting?
> >
> > Meeting was cancelled, but I raised the issue here:
> > https://gitlab.khronos.org/egl/API/merge_requests/17
> >
> > Right now we have ARM saying they return false + error and NVIDIA saying
> > they return true + no error and that ARM has a bug.
> > I think another party adding their opinion might nudge it forward :)
> >
> Fwiw I put forward a suggestion to add a workaround in the
> Android/CrOS libEGL wrapper for the ARM drivers.
> Although one could also consider the Nvidia/Mesa drivers as
> non-compliant and add a workaround for those instead.
>
> I have to admit it's not pretty, but it seems consistent with all the
> other workarounds in the wrapper.
>
> As Eric said - input from another party should, would be great.

Thanks for the update.

Still, Mesa is crashing right now, which is probably the least
conformant behavior. Do you think we could go with first version on
Erik's patch to just return EGL_TRUE?

Best regards,
Tomasz