[Mesa-dev] [PATCH v3] egl/android: support for EGL_KHR_partial_update
Emil Velikov
emil.l.velikov at gmail.com
Mon Jun 5 16:04:28 UTC 2017
Hi Harish,
There's a handful of comments, most of which style related nitpicks.
Couple that apply through the patch:
- rewrap the comments to utilise ~80 columns.
- please use consistent variable names - fooBar is not used in src/egl.
Splitting the core implementation and Android code into separate
patches, would be great but optional.
On 5 June 2017 at 15:07, Harish Krupo <harish.krupo.kps at intel.com> wrote:
> This patch adds support for the EGL_KHR_partial_update extension for
> android platform. It passes 36/37 tests in dEQP for EGL_KHR_partial_update.
> 1 test not supported.
>
> v2: add fallback for eglSetDamageRegionKHR (Tapani)
>
> v3: The native_window_set_surface_damage call is available only from
> Android version 6.0. Reintroduce the ANDROID_VERSION guard and
> advertise extension only if version is >= 6.0. (Emil Velikov)
>
> Signed-off-by: Harish Krupo <harish.krupo.kps at intel.com>
> ---
> Android.common.mk | 3 +-
> Android.mk | 2 +
> src/egl/drivers/dri2/egl_dri2.c | 12 +++++
> src/egl/drivers/dri2/egl_dri2.h | 4 ++
> src/egl/drivers/dri2/platform_android.c | 37 ++++++++++++++
> src/egl/main/eglapi.c | 87 +++++++++++++++++++++++++++++++++
> src/egl/main/eglapi.h | 2 +
> src/egl/main/egldisplay.h | 1 +
> src/egl/main/eglentrypoint.h | 1 +
> src/egl/main/eglfallbacks.c | 1 +
> src/egl/main/eglsurface.c | 8 +++
> src/egl/main/eglsurface.h | 12 +++++
> 12 files changed, 169 insertions(+), 1 deletion(-)
>
> diff --git a/Android.common.mk b/Android.common.mk
> index d9b198a956..4777ea8bd9 100644
> --- a/Android.common.mk
> +++ b/Android.common.mk
> @@ -39,7 +39,8 @@ LOCAL_CFLAGS += \
> -Wno-initializer-overrides \
> -Wno-mismatched-tags \
> -DPACKAGE_VERSION=\"$(MESA_VERSION)\" \
> - -DPACKAGE_BUGREPORT=\"https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa\"
> + -DPACKAGE_BUGREPORT=\"https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa\" \
> + -DANDROID_VERSION=0x0$(MESA_ANDROID_MAJOR_VERSION)0$(MESA_ANDROID_MINOR_VERSION)
>
On second thought, this will be iffy with Android O and later, since
the major is a letter.
The newly introduced ANDROID_API_LEVEL might be a better fit?
Apologies for misleading you :-\
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1464,6 +1464,17 @@ dri2_swap_buffers_region(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf,
> }
>
> static EGLBoolean
> +dri2_set_damage_region(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf,
> + EGLint *rects, EGLint n_rects)
> +{
> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> + if (dri2_dpy->vtbl->set_damage_region)
> + return dri2_dpy->vtbl->set_damage_region(drv, dpy, surf, rects, n_rects);
> +
> + return EGL_FALSE;
Please add a fallback entry in egl_dri2_fallbacks.h use it in the
other platforms.
Then this check becomes obsolete.
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -651,6 +651,39 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw)
> return EGL_TRUE;
> }
>
> +static EGLBoolean
> +droid_set_damage_region(_EGLDriver *drv,
> + _EGLDisplay *disp,
> + _EGLSurface *draw, const EGLint* rects, EGLint n_rects)
> +{
> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw);
> + android_native_rect_t* droid_rects = NULL;
Nit: add blank line.
> + if (n_rects != 0)
> + droid_rects = (android_native_rect_t *)
Please drop the cast - this is not a C++ source.
> + calloc(n_rects, sizeof(android_native_rect_t));
> +
> + EGLint surfWidth = dri2_surf->base.Width;
> + EGLint surfHeight = dri2_surf->base.Height;
> + EGLint dIndex;
> +
dIndex is not needed - use "EGLint i = 0; i < n_rects; += 4" in the
loop condition.
> + for (dIndex = 0; dIndex < n_rects; dIndex++) {
> + EGLint i = dIndex * 4;
> + droid_rects[dIndex].left = rects[i]; // left == x
> + droid_rects[dIndex].bottom = rects[i + 1]; // bottom == y
> + droid_rects[dIndex].right = rects[i] + rects[i + 2]; // left + width
> + droid_rects[dIndex].top = rects[i + 1] + rects[i + 3]; // bottom + height
Comments don't help much. Please drop.
> + }
> +
> +#if ANDROID_VERSION >= 0x600
> + native_window_set_surface_damage(dri2_surf->window, droid_rects, n_rects);
Isolating the most important function like this is a bad idea. Please
wrap the whole of droid_set_damage_region.
> +#endif
> +
> + free(droid_rects);
> +
> + return EGL_TRUE;
> +}
> +
> static _EGLImage *
> droid_create_image_from_prime_fd_yuv(_EGLDisplay *disp, _EGLContext *ctx,
> struct ANativeWindowBuffer *buf, int fd)
> @@ -1101,6 +1134,7 @@ static struct dri2_egl_display_vtbl droid_display_vtbl = {
> .swap_buffers = droid_swap_buffers,
> .swap_buffers_with_damage = dri2_fallback_swap_buffers_with_damage,
> .swap_buffers_region = dri2_fallback_swap_buffers_region,
> + .set_damage_region = droid_set_damage_region,
With the above suggestions this becomes
#if ANDROID_API_LEVEL >= 23
.set_damage_region = droid_set_damage_region,
#else
.set_damage_region = dri2_fallback_set_damage_region,
#endif
> @@ -1281,6 +1300,74 @@ eglSwapBuffersWithDamageKHR(EGLDisplay dpy, EGLSurface surface,
> return _eglSwapBuffersWithDamageCommon(disp, surf, rects, n_rects);
> }
>
> +/*
Nit: make this /**
> + * If the width of the passed rect is greater than the surface's
> + * width then it is clamped to the width of the surface. Same with
> + * height.
> + */
> +
> +static void
> +_eglSetDamageRegionKHRClampRects(_EGLDisplay* disp, _EGLSurface* surf,
> + EGLint *rects, EGLint n_rects)
> +{
> + EGLint i;
> + EGLint surfHeight = surf->Height;
> + EGLint surfWidth = surf->Width;
> +
> + for (i = 0; i < (4 * n_rects); i += 4) {
Do we really need the "4 *" here?
> + EGLint x, y, rectWidth, rectHeight;
> + x = rects[i];
> + y = rects[i + 1];
> + rectWidth = rects[i + 2];
> + rectHeight = rects[i + 3];
> +
> + if (rectWidth + x > surfWidth)
Math can overflow.
> + rects[i + 2] = surfWidth - x;
> +
> + if (rectHeight + y > surfHeight)
Ditto.
> + rects[i + 3] = surfHeight - y;
> + }
> +}
> +
> +static EGLBoolean EGLAPIENTRY
> +eglSetDamageRegionKHR(EGLDisplay dpy, EGLSurface surface,
> + EGLint *rects, EGLint n_rects)
> +{
> + _EGLDisplay *disp = _eglLockDisplay(dpy);
> + _EGLSurface *surf = _eglLookupSurface(surface, disp);
> + _EGL_FUNC_START(disp, EGL_OBJECT_SURFACE_KHR, surf, EGL_FALSE);
> + _EGLContext *ctx = _eglGetCurrentContext();
> + _EGLDriver *drv;
> + EGLBoolean ret;
> + _EGL_CHECK_SURFACE(disp, surf, EGL_FALSE, drv);
> +
> + if (_eglGetContextHandle(ctx) == EGL_NO_CONTEXT ||
> + surf->Type != EGL_WINDOW_BIT || ctx->DrawSurface != surf ||
Nit: Group the error conditions
if (not_current_surface ||
not_postable ||
not_buffer_destroyed)
if (_eglGetContextHandle(ctx) == EGL_NO_CONTEXT || ctx->DrawSurface
!= surf ||
surf->Type != EGL_WINDOW_BIT ||
> + surf->SwapBehavior != EGL_BUFFER_DESTROYED)
> + RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_FALSE);
> +
> + /* If the damage region is already set between
> + * frame boundaries, throw bad access error
> + */
> + if (surf->SetDamageRegion)
> + RETURN_EGL_ERROR(disp, EGL_BAD_ACCESS, EGL_FALSE);
> +
> + /* If the buffer age has not been queried before
> + * setting the damage region, between
> + * frame boundaries, throw bad access error
> + */
Not sure these comments bring much. Please rewrap if you to prefer to keep.
Regards,
Emil
More information about the mesa-dev
mailing list