Bug 144685 – [EGL] Move EGLDisplay handling to PlatformDisplay and remove PlatformDisplay definition from GLDefs.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

And remove the PlatformDisplay definition from platform/graphics/opengl/GLDefs.h. The EGL display is also initialized in multiple places, and could be unified in PlatformDisplay.

[Carlos Garcia Campos]

Created attachment 252469 [details]
Patch

[Carlos Garcia Campos]

Created attachment 252470 [details]
Try to fix EFL build

[Martin Robinson]

Comment on attachment 252470 [details]
Try to fix EFL build

View in context: https://bugs.webkit.org/attachment.cgi?id=252470&action=review

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:139
> +#if USE(OPENGL_ES_2)
> +    static const EGLenum eglAPIVersion = EGL_OPENGL_ES_API;
> +#else
> +    static const EGLenum eglAPIVersion = EGL_OPENGL_API;
> +#endif
> +    if (eglBindAPI(eglAPIVersion) == EGL_FALSE) {
> +        LOG_ERROR("Failed to set EGL API(%d).", eglGetError());
> +        terminateEGLDisplay();
> +        return;
> +    }

This needs to be called once per program, not once per display. Perhaps you could surround this with std::once.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:153
> -    m_glxPixmap = glXCreateGLXPixmap(m_sharedDisplay, visualInfo.get(), m_pixmap);
> +    m_glxPixmap = glXCreateGLXPixmap(X11Helper::nativeDisplay(), visualInfo.get(), m_pixmap);

This is GLX-related, so perhaps you could update the bug title and ChangeLog slightly?

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 252470 [details]
> Try to fix EFL build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252470&action=review
> 
> > Source/WebCore/platform/graphics/PlatformDisplay.cpp:139
> > +#if USE(OPENGL_ES_2)
> > +    static const EGLenum eglAPIVersion = EGL_OPENGL_ES_API;
> > +#else
> > +    static const EGLenum eglAPIVersion = EGL_OPENGL_API;
> > +#endif
> > +    if (eglBindAPI(eglAPIVersion) == EGL_FALSE) {
> > +        LOG_ERROR("Failed to set EGL API(%d).", eglGetError());
> > +        terminateEGLDisplay();
> > +        return;
> > +    }
> 
> This needs to be called once per program, not once per display. Perhaps you
> could surround this with std::once.

Display is already a single instance, so this will be called only once per display, per process.

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:153
> > -    m_glxPixmap = glXCreateGLXPixmap(m_sharedDisplay, visualInfo.get(), m_pixmap);
> > +    m_glxPixmap = glXCreateGLXPixmap(X11Helper::nativeDisplay(), visualInfo.get(), m_pixmap);
> 
> This is GLX-related, so perhaps you could update the bug title and ChangeLog
> slightly?

But nothing has actually changed here, I can add and remove PlatformDisplay from GLDefs.h

[Martin Robinson]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 252470 [details]
> > Try to fix EFL build
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=252470&action=review
> > 
> > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:139
> > > +#if USE(OPENGL_ES_2)
> > > +    static const EGLenum eglAPIVersion = EGL_OPENGL_ES_API;
> > > +#else
> > > +    static const EGLenum eglAPIVersion = EGL_OPENGL_API;
> > > +#endif
> > > +    if (eglBindAPI(eglAPIVersion) == EGL_FALSE) {
> > > +        LOG_ERROR("Failed to set EGL API(%d).", eglGetError());
> > > +        terminateEGLDisplay();
> > > +        return;
> > > +    }
> > 
> > This needs to be called once per program, not once per display. Perhaps you
> > could surround this with std::once.
> 
> Display is already a single instance, so this will be called only once per
> display, per process.

eglBindAPI isn't supposed to be called once per-display. I supposed to be called once.

> 
> > > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:153
> > > -    m_glxPixmap = glXCreateGLXPixmap(m_sharedDisplay, visualInfo.get(), m_pixmap);
> > > +    m_glxPixmap = glXCreateGLXPixmap(X11Helper::nativeDisplay(), visualInfo.get(), m_pixmap);
> > 
> > This is GLX-related, so perhaps you could update the bug title and ChangeLog
> > slightly?
> 
> But nothing has actually changed here, I can add and remove PlatformDisplay
> from GLDefs.h

I'm not sure I understand what you mean, but the change is so small that you can probably skip mentioning GLX in the ChangeLog if you don't think it matters.

[Carlos Garcia Campos]

Created attachment 252475 [details]
Patch for landing

[Carlos Garcia Campos]

Created attachment 252476 [details]
Patch for landing

[Carlos Garcia Campos]

Created attachment 252478 [details]
Patch for landing

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 252470 [details]
> > > Try to fix EFL build
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=252470&action=review
> > > 
> > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:139
> > > > +#if USE(OPENGL_ES_2)
> > > > +    static const EGLenum eglAPIVersion = EGL_OPENGL_ES_API;
> > > > +#else
> > > > +    static const EGLenum eglAPIVersion = EGL_OPENGL_API;
> > > > +#endif
> > > > +    if (eglBindAPI(eglAPIVersion) == EGL_FALSE) {
> > > > +        LOG_ERROR("Failed to set EGL API(%d).", eglGetError());
> > > > +        terminateEGLDisplay();
> > > > +        return;
> > > > +    }
> > > 
> > > This needs to be called once per program, not once per display. Perhaps you
> > > could surround this with std::once.
> > 
> > Display is already a single instance, so this will be called only once per
> > display, per process.
> 
> eglBindAPI isn't supposed to be called once per-display. I supposed to be
> called once.

There's only one display, so it will be called once. Anyway, I've moved the egl initializations repeated in several places here, so if it was wrong it can be fixed in a follow up patch, this patch shouldn't change any behaviour. But I insist, this is called only once per process.

[Martin Robinson]

(In reply to comment #9)

> There's only one display, so it will be called once. Anyway, I've moved the
> egl initializations repeated in several places here, so if it was wrong it
> can be fixed in a follow up patch, this patch shouldn't change any
> behaviour. But I insist, this is called only once per process.

Okay. I misunderstood what you wrote earlier. Sorry about that. This sounds fine.

[Carlos Garcia Campos]

Committed r183868: <http://trac.webkit.org/changeset/183868>