[Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
This fixes crash due to NULL window when swap interval is set for pbuffer surface. Test: CtsDisplayTestCases pass Signed-off-by: samiuddi --- 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; surf->SwapInterval = interval; -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov wrote: > > On 30 August 2018 at 11:41, Eric Engestrom 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 > >> wrote: > >> > > >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov > >> > wrote: > >> > > > >> > > On 5 July 2018 at 17:17, Eric Engestrom > >> > > wrote: > >> > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: > >> > > >> On 5 July 2018 at 10:53, Eric Engestrom > >> > > >> 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 > >> > > >> >> --- > >> > > >> >> > >> > > >> >> 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 -
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
On Monday, 2018-09-03 14:59:49 +0900, Tomasz Figa wrote: > On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov > wrote: > > > > On 30 August 2018 at 11:41, Eric Engestrom 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 > > >> wrote: > > >> > > > >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov > > >> > wrote: > > >> > > > > >> > > On 5 July 2018 at 17:17, Eric Engestrom > > >> > > wrote: > > >> > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: > > >> > > >> On 5 July 2018 at 10:53, Eric Engestrom > > >> > > >> 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 > > >> > > >> >> --- > > >> > > >> >> > > >> > > >> >> 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
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
On 3 September 2018 at 08:14, Eric Engestrom wrote: > On Monday, 2018-09-03 14:59:49 +0900, Tomasz Figa wrote: >> On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov >> wrote: >> > >> > On 30 August 2018 at 11:41, Eric Engestrom >> > 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 >> > >> wrote: >> > >> > >> > >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov >> > >> > wrote: >> > >> > > >> > >> > > On 5 July 2018 at 17:17, Eric Engestrom >> > >> > > wrote: >> > >> > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: >> > >> > > >> On 5 July 2018 at 10:53, Eric Engestrom >> > >> > > >> 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 >> > >> > > >> >> --- >> > >> > > >> >> >> > >> > > >> >> 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 sa
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
Hi Erik, Emil, Eric, On Tue, Jul 10, 2018 at 12:41 AM Erik Faye-Lund wrote: > > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov wrote: > > > > On 5 July 2018 at 17:17, Eric Engestrom wrote: > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: > > >> On 5 July 2018 at 10:53, Eric Engestrom 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 > > >> >> --- > > >> >> > > >> >> 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? Best regards, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
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 wrote: > > > > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov > > wrote: > > > > > > On 5 July 2018 at 17:17, Eric Engestrom wrote: > > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: > > > >> On 5 July 2018 at 10:53, Eric Engestrom > > > >> 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 > > > >> >> --- > > > >> >> > > > >> >> 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 :) > > Best regards, > Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
On 30 August 2018 at 11:41, Eric Engestrom 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 wrote: >> > >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov >> > wrote: >> > > >> > > On 5 July 2018 at 17:17, Eric Engestrom wrote: >> > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: >> > > >> On 5 July 2018 at 10:53, Eric Engestrom >> > > >> 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 >> > > >> >> --- >> > > >> >> >> > > >> >> 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. HTH Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
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 > --- > > 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; > > surf->SwapInterval = interval; > -- > 2.7.4 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
On 5 July 2018 at 10:53, Eric Engestrom 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 >> --- >> >> 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. 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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: > On 5 July 2018 at 10:53, Eric Engestrom 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 > >> --- > >> > >> 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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
On 5 July 2018 at 17:17, Eric Engestrom wrote: > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: >> On 5 July 2018 at 10:53, Eric Engestrom 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 >> >> --- >> >> >> >> 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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.
On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov wrote: > > On 5 July 2018 at 17:17, Eric Engestrom wrote: > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote: > >> On 5 July 2018 at 10:53, Eric Engestrom 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 > >> >> --- > >> >> > >> >> 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
