[Mesa-dev] [PATCH 2/2] egl: Check if API is supported when using eglBindAPI.

[Manolova, Plamena]

Hi Ian,
Thanks for reviewing!

On Wed, May 18, 2016 at 4:33 PM, Ian Romanick <idr at freedesktop.org> wrote:

> On 05/17/2016 09:35 AM, Plamena Manolova wrote:
> > According to the EGL specifications before binding an API
> > we must check whether it's supported first. If not eglBindAPI
> > should return EGL_FALSE and generate a EGL_BAD_PARAMETER error.
>
> Can you provide a spec quotation?
>

https://www.khronos.org/registry/egl/sdk/docs/man/html/eglBindAPI.xhtml

"EGL_BAD_PARAMETER is generated if api is not one of the accepted tokens,
or if the
specified client API is not supported by the EGL implementation."

> Signed-off-by: Plamena Manolova <plamena.manolova at intel.com>
> > ---
> >  src/egl/main/eglcurrent.h | 33 ++++++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/egl/main/eglcurrent.h b/src/egl/main/eglcurrent.h
> > index 1e386ac..f2e19cc 100644
> > --- a/src/egl/main/eglcurrent.h
> > +++ b/src/egl/main/eglcurrent.h
> > @@ -32,7 +32,8 @@
> >  #include "c99_compat.h"
> >
> >  #include "egltypedefs.h"
> > -
> > +#include "eglglobals.h"
> > +#include "egldisplay.h"
> >
> >  #ifdef __cplusplus
> >  extern "C" {
> > @@ -62,14 +63,40 @@ struct _egl_thread_info
> >     EGLint CurrentAPIIndex;
> >  };
> >
> > -
> > +static inline EGLBoolean
> > +_eglDisplaySupportsApi(_EGLDisplay *dpy, EGLenum api)
>
> Since this is only used internally, please use bool/true/false.  Based
> on my comments at the bottom, I think this function should go directly
> in eglapi.c.
>
> > +{
>
> This is a really complex way of doing something quite simple.  How about.
>
>    unsigned api_bit;
>
>    if (!dpy->Initialized)
>       return false:
>
>    switch (api) {
>    case EGL_OPENGL_API:
>       api_bit = EGL_OPENGL_BIT;
>       break;
>    case EGL_OPENGL_ES_API:
>       api_bit = EGL_OPENGL_ES_BIT |
>                 EGL_OPENGL_ES2_BIT |
>                 EGL_OPENGL_ES3_BIT_KHR;
>       break;
>    case EGL_OPENVG_API:
>       api_bit = EGL_OPENVG_BIT;
>       break;
>    default:
>       api_bit = 0;
>       break;
>    }
>
>    return (dpy->ClientAPIs & api_bit) != 0;


I'll make those changes.


>
> > +   if (!dpy->Initialized) {
> > +      return EGL_FALSE;
> > +   } else if (api == EGL_OPENGL_API && dpy->ClientAPIs &
> EGL_OPENGL_BIT) {
> > +      return EGL_TRUE;
> > +   } else if (api == EGL_OPENGL_ES_API &&
> > +      (dpy->ClientAPIs & EGL_OPENGL_ES_BIT ||
> > +       dpy->ClientAPIs & EGL_OPENGL_ES2_BIT ||
> > +       dpy->ClientAPIs & EGL_OPENGL_ES3_BIT_KHR)) {
> > +      return EGL_TRUE;
> > +   } else if (api == EGL_OPENVG_API && dpy->ClientAPIs &
> EGL_OPENVG_BIT) {
> > +      return EGL_TRUE;
> > +   } else {
> > +      return EGL_FALSE;
> > +   }
> > +}
> >  /**
> >   * Return true if a client API enum is recognized.
> >   */
> >  static inline EGLBoolean
> >  _eglIsApiValid(EGLenum api)
>
> There's only one user of this function, and I suspect there will only
> every be that one user.  I would do a first patch that moves this
> function into eglapi.c... maybe even just move this function into the
> only caller.  Then this patch would change the way it works.
>
>
Makes sense I'll go ahead and move it to the actual caller.


> >  {
> > -   return (api >= _EGL_API_FIRST_API && api <= _EGL_API_LAST_API);
> > +   _EGLDisplay *dpy = _eglGlobal.DisplayList;
> > +
> > +   while (dpy) {
> > +      if (_eglDisplaySupportsApi(dpy, api) == EGL_TRUE)
>
> Even without changing _eglDisplaySupportsApi to return bool, you can
> drop the '== EGL_TRUE'.
>
> > +         return EGL_TRUE;
> > +
> > +      dpy = dpy->Next;
> > +   }
> > +
> > +   return EGL_FALSE;
> >  }
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160518/c1cddb47/attachment-0001.html>