[Mesa-dev] [PATCH] egl/dri2: Implement swapInterval fallback in a conformant way

[Eric Engestrom]

On 10 August 2017 06:43:45 BST, Tomasz Figa <tfiga at chromium.org> wrote:
> dri2_fallback_swap_interval() currently used to stub out swap interval
> support in Android backend does nothing besides returning EGL_FALSE.
> This causes at least one known application (Android Snapchat) to fail
> due to an unexpected error and my loose interpretation of the EGL 1.5
> specification justifies it. Relevant quote below:
> 
>     The function
> 
>         EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval);
> 
>    specifies the minimum number of video frame periods per buffer swap
> for the draw surface of the current context, for the current rendering
>     API. [...]
> 
>    The parameter interval specifies the minimum number of video frames
>     that are displayed before a buffer swap will occur. The interval
>     specified by the function applies to the draw surface bound to the
>     context that is current on the calling thread. [...] interval is
>     silently clamped to minimum and maximum implementation dependent
>     values before being stored; these values are defined by EGLConfig
>     attributes EGL_MIN_SWAP_INTERVAL and EGL_MAX_SWAP_INTERVAL
>     respectively.
> 
>     The default swap interval is 1.
> 
> Even though it does not specify the exact behavior if the platform
> does
> not support changing the swap interval, the default assumed state is
> the
> swap interval of 1, which I interpret as a value that
> eglSwapInterval()
> should succeed if called with, even if there is no ability to change
> the
> interval (but there is no change requested). Moreover, since the
> behavior is defined to clamp the requested value to minimum and
> maximum
> and at least the default value of 1 must be present in the range, the
> implementation might be expected to have a valid range, which in case
> of
> the feature being unsupported, would correspond to {1} and any request
> might be expected to be clamped to this value.
> 
> This is further confirmed by the code in _eglSwapInterval() in
> src/egl/main/eglsurface.c, which is the default fallback
> implementation
> for EGL drivers not implementing its own. The problem with it is that
> the DRI2 EGL driver provides its own implementation that calls into
> platform backends, so we cannot just simply fall back to it.
> 
> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
> ---
>  src/egl/drivers/dri2/egl_dri2.c           | 12 ++++++++++++
>  src/egl/drivers/dri2/egl_dri2_fallbacks.h |  9 ++++++++-
>  src/egl/drivers/dri2/platform_x11.c       |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c
> b/src/egl/drivers/dri2/egl_dri2.c
> index f0d1ded408..686dd68d29 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -630,6 +630,18 @@ dri2_setup_screen(_EGLDisplay *disp)
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>     unsigned int api_mask;
>  
> +   /*
> +    * EGL 1.5 specification defines the default value to 1. Moreover,
> +    * eglSwapInterval() is required to clamp requested value to the
> supported
> +    * range. Since the default value is implicitly assumed to be
> supported,
> +    * use it as both minimum and maximum for the platforms that do
> not allow
> +    * changing the interval. Platforms, which allow it (e.g. x11,
> wayland)
> +    * override these values already.
> +    */
> +   dri2_dpy->min_swap_interval = 1;
> +   dri2_dpy->max_swap_interval = 1;
> +   dri2_dpy->default_swap_interval = 1;
> +
>     if (dri2_dpy->image_driver) {
>   api_mask = dri2_dpy->image_driver->getAPIMask(dri2_dpy->dri_screen);
>     } else if (dri2_dpy->dri2) {
> diff --git a/src/egl/drivers/dri2/egl_dri2_fallbacks.h
> b/src/egl/drivers/dri2/egl_dri2_fallbacks.h
> index 604db881a8..c70c686f8d 100644
> --- a/src/egl/drivers/dri2/egl_dri2_fallbacks.h
> +++ b/src/egl/drivers/dri2/egl_dri2_fallbacks.h
> @@ -59,7 +59,14 @@ static inline EGLBoolean
>  dri2_fallback_swap_interval(_EGLDriver *drv, _EGLDisplay *dpy,
>                              _EGLSurface *surf, EGLint interval)
>  {
> -   return EGL_FALSE;
> +   if (interval > surf->Config->MaxSwapInterval)
> +      interval = surf->Config->MaxSwapInterval;
> +   else if (interval < surf->Config->MinSwapInterval)
> +      interval = surf->Config->MinSwapInterval;
> +
> +   surf->SwapInterval = interval;
> +
> +   return EGL_TRUE;

Agreed with the interpretation, but if memory serves (on my phone on a plane right now) I already took care of clamping and setting the value one layer above, so the only change needed is s/EGL_FALSE/EGL_TRUE/ in this function.

Look for a commit of mine (`git log --author=engestrom -- src/egl/`) about 3 weeks ago.

Cheers,
  Eric

>  }
>  
>  static inline EGLBoolean
> diff --git a/src/egl/drivers/dri2/platform_x11.c
> b/src/egl/drivers/dri2/platform_x11.c
> index 4610ec579f..97cdd09078 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -1283,6 +1283,7 @@ dri2_x11_setup_swap_interval(struct
> dri2_egl_display *dri2_dpy)
>      */
>     dri2_dpy->min_swap_interval = 0;
>     dri2_dpy->max_swap_interval = 0;
> +   dri2_dpy->default_swap_interval = 0;
>  
>     if (!dri2_dpy->swap_available)
>        return;