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

[Emil Velikov]

On 10 August 2017 at 13:51, Tomasz Figa <tfiga at chromium.org> wrote:
> On Thu, Aug 10, 2017 at 9:40 PM, Eric Engestrom <eric at engestrom.ch> wrote:
>> 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.
>>
Eric, you beat me to it. I thought you're on holidays :-)

>> Look for a commit of mine (`git log --author=engestrom -- src/egl/`) about 3 weeks ago.
>
> Good catch, thanks for pointing out. I should have checked upstream
> first. Looks like the rebase of our branch was just few days before
> that commit. :)
>
> Actually we shouldn't need that fallback at all with your changes, if
> we make sure the we have surf->SwapInterval, surf->MaxSwapInterval and
> surf->MinSwapInterval initialized to 1.
>
Do you know of any Android apps that expect swap interval greater than 1?
I'm wondering if it will be useful to report a case when application
has requests interval > 1, while platform X has no implementation.

Note: I have a couple of small patches which will cause some rebase
conflicts. Will push those in a moment.

Thanks
Emil