On Thu, Aug 10, 2017 at 9:40 PM, Eric Engestrom <e...@engestrom.ch> wrote:
> On 10 August 2017 06:43:45 BST, Tomasz Figa <tf...@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 <tf...@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.
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.
Best regards,
Tomasz
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
