Bug 108034 – [EFL][WebGL] Add proper checks to enable GraphicsSurface usage on EGL without XCompositeWindow.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kalyan]

Currently, we set GRAPHICS_SURFACE to true if support for XCompositeWindow is identified during compile time. This is ok, when using GLX. With EGL and GLES2.0 we want to use XPixmap or some kind of Drm Buffer(if supported) instead of XCompositeWindow. We could support both Window and Pixmap for EGL but I think it is unnecessary overhead and we could just use XPixmap for both desktop and when using GLES2.0. Thus, it would be be better to introduce a new macro to identify XCompsiteWindow support and alwayse set GraphicsSurface.

[Kalyan]

Created attachment 190030 [details]
Patch

[Laszlo Gombos]

Comment on attachment 190030 [details]
Patch

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

For now we should assume XCompositeWindow for USE(GLX). I would not yet expose the guard for XCOMPOSITE to the common code.

> Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:31
> -#if HAVE(GLX)
> +#if USE(GLX)

I do not think the name change here is necessary. I would remove this change.

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigSelector.h:154
> -#if USE(GRAPHICS_SURFACE)
> +#if USE(XCOMPOSITE) || (PLATFORM(QT) && USE(GRAPHICS_SURFACE))

I would just use USE(GRAPHICS_SURFACE) && USE(GLX) as a guard and set WTF_USE_GLX for Qt (perhaps in Platform.h or in the Qt build system).

> Source/WebCore/platform/graphics/surfaces/glx/X11Helper.cpp:186
> -#if USE(GRAPHICS_SURFACE)
> +#if USE(XCOMPOSITE) || (PLATFORM(QT) && USE(GRAPHICS_SURFACE))

Ditto.

> Source/WebCore/platform/graphics/surfaces/glx/X11Helper.h:36
> +#if USE(XCOMPOSITE) || (PLATFORM(QT) && USE(GRAPHICS_SURFACE))

USE(GLX)

> Source/cmake/OptionsEfl.cmake:189
> +        set(USE_GRAPHICS_SURFACE 1)

I think we should move this rule out from the build system into Platform.h.

> Source/cmake/OptionsEfl.cmake:205
> +             set(USE_GRAPHICS_SURFACE 1)

Ditto we should remove this line and move the rule to Platform.h. Would the following work in Platform.h ? 

#if (USE(EGL) && PLATFORM(EFL)) || (USE(GLX) && (PLATFORM(EFL) || PLATFORM(QT)))
#define WTF_USE_GRAPHICS_SURFACE 1
#endif

Fail the build if there is no Xcomposite and add a FIXME to add support for it later.

# FIXME: Add support for NOT X11_Xcomposite for GLX
if (NOT X11_Xcomposite_FOUND OR NOT X11_Xrender_FOUND)
message( FATAL_ERROR "GLX support requires X11_Xcomposite.")
endif ()

> ChangeLog:8
> +        Add a macro to identify XComposite ext support.

Let's delay adding the macro until we have code supporting the !USE(XCOMPOSITE) case.

[Kalyan]

updated the bug title to reflect the changes.

[Kalyan]

Created attachment 190879 [details]
patchv2

[Kalyan]

(In reply to comment #2)
> (From update of attachment 190030 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190030&action=review
> 
> For now we should assume XCompositeWindow for USE(GLX). I would not yet expose the guard for XCOMPOSITE to the common code.
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:31
> > -#if HAVE(GLX)
> > +#if USE(GLX)
> 

I would prefer to change it to USE as even though we have the support for GLX we dont use it if one builds with EGL support during compile time. This would also sync in with other places with similar checks(i.e our own code and also on GTK side). Any thoughts?

> 
> Let's delay adding the macro until we have code supporting the !USE(XCOMPOSITE) case.

Done.

[Build Bot]

Comment on attachment 190879 [details]
patchv2

Attachment 190879 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16763197

New failing tests:
editing/selection/selection-invalid-offset.html

[Laszlo Gombos]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 190030 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190030&action=review
> > 
> > For now we should assume XCompositeWindow for USE(GLX). I would not yet expose the guard for XCOMPOSITE to the common code.
> > 
> > > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:31
> > > -#if HAVE(GLX)
> > > +#if USE(GLX)
> > 
> 
> I would prefer to change it to USE as even though we have the support for GLX we dont use it if one builds with EGL support during compile time. This would also sync in with other places with similar checks(i.e our own code and also on GTK side). Any thoughts?

Ok, I have not realized that we're already using _both_ HAVE(GLX) and USE(GLX) in the code. Standardizing on USE(GLX) make sense to me.

[Laszlo Gombos]

Comment on attachment 190879 [details]
patchv2

r=me. 

We should move X11Helper our form the glx directory as well, I find it a bit confusing. Can you file a bug for that ?

[Kalyan]

Created attachment 191156 [details]
patchforlanding

[WebKit Review Bot]

Comment on attachment 191156 [details]
patchforlanding

Clearing flags on attachment: 191156

Committed r144602: <http://trac.webkit.org/changeset/144602>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kalyan]

(In reply to comment #8)
> (From update of attachment 190879 [details])
> r=me. 
> 
> We should move X11Helper our form the glx directory as well, I find it a bit confusing. Can you file a bug for that ?

Created a new issue for this: 
https://bugs.webkit.org/show_bug.cgi?id=111347