On Wed 13 May 2015, Marek Olšák wrote:
> From: Marek Olšák <marek.ol...@amd.com>
>
> ---
> src/egl/main/eglapi.c | 14 +++++++++++++-
> src/egl/main/eglapi.h | 2 +-
> src/egl/main/eglsync.c | 2 +-
> src/egl/main/eglsync.h | 2 +-
> 4 files changed, 16 insertions(+), 4 deletions(-)
>
I see two issues below.
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index 544f7e4..6457798 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -1558,6 +1559,17 @@ eglGetSyncAttribKHR(EGLDisplay dpy, EGLSync sync,
> EGLint attribute, EGLint *valu
> }
>
>
> +EGLBoolean EGLAPIENTRY
> +eglGetSyncAttribKHR(EGLDisplay dpy, EGLSync sync, EGLint attribute, EGLint
> *value)
> +{
> + EGLAttrib attrib = *value;
> + EGLBoolean result = eglGetSyncAttrib(dpy, sync, attribute, &attrib);
> +
> + *value = attrib;
> + return result;
> +}
This change violates the EGL_KHR_fence_sync spec, which says this
regarding eglGetSyncAttribKHR:
If any error occurs, <*value> is not modified.
I think it's a good idea to add this quote to the function body so that
future devs don't accidentally make the same mistake.
> diff --git a/src/egl/main/eglapi.h b/src/egl/main/eglapi.h
> index 44f589d..fdd3b05 100644
> --- a/src/egl/main/eglapi.h
> +++ b/src/egl/main/eglapi.h
> @@ -91,7 +91,7 @@ typedef EGLBoolean (*DestroySyncKHR_t)(_EGLDriver *drv,
> _EGLDisplay *dpy, _EGLSy
> typedef EGLint (*ClientWaitSyncKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy,
> _EGLSync *sync, EGLint flags, EGLTime timeout);
> typedef EGLint (*WaitSyncKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync
> *sync);
> typedef EGLBoolean (*SignalSyncKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy,
> _EGLSync *sync, EGLenum mode);
> -typedef EGLBoolean (*GetSyncAttribKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy,
> _EGLSync *sync, EGLint attribute, EGLint *value);
> +typedef EGLBoolean (*GetSyncAttribKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy,
> _EGLSync *sync, EGLint attribute, EGLAttrib *value);
I think changing this change is potentially dangerous, because
post-patch GetSyncAttribKHR_t has the signature of eglGetSyncAttrib and
*not* eglGetSyncAttribKHR. The code will compile, but this discrepancy
looks like a honeypot for accidental function misuse.
And...
> #ifdef EGL_NOK_swap_region
> diff --git a/src/egl/main/eglsync.c b/src/egl/main/eglsync.c
> index 205cdc0..a09d991 100644
> --- a/src/egl/main/eglsync.c
> +++ b/src/egl/main/eglsync.c
> @@ -142,7 +142,7 @@ _eglInitSync(_EGLSync *sync, _EGLDisplay *dpy, EGLenum
> type,
>
> EGLBoolean
> _eglGetSyncAttribKHR(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
> - EGLint attribute, EGLint *value)
> + EGLint attribute, EGLAttrib *value)
I have the same issue here. Let's avoid introducing confusing
discrepancies between function names and their signatures. Just name
this one and the typedef to '_eglGetSyncAttrib' and 'GetSyncAttrib_t'.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
