Bug 113359 – [EFL][EGL] Add support for creating offscreen surface.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

Add support for using EGLPixmapSurface as an offscreensurface.

[Kalyan]

Created attachment 195244 [details]
patch

[Kalyan]

Created attachment 195381 [details]
patchv2

[Noam Rosenthal]

Comment on attachment 195381 [details]
patchv2

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

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:66
>          configAttributeList[11] = m_attributes & GLPlatformSurface::SupportAlpha ? 8 : 0;
>          configAttributeList[13] = EGL_PIXMAP_BIT;

I know this code was there before, but this is extremely bug prone. I'd prefer if instead of using a static array and changing it here with a magic index you'd just generate the array here on the fly.

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:77
>          configAttributeList[11] = m_attributes & GLPlatformSurface::SupportAlpha ? 8 : 0;
>          configAttributeList[13] = EGL_WINDOW_BIT;

ditto

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:112
> +    EGLint numConfigs, i;

Two lines

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:119
> +        return config;

return 0;

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:122
> +    configs = static_cast<EGLConfig*>(malloc(numConfigs * sizeof(EGLConfig)));

configs = new EGLConfig[numConfigs];
C++ please :)

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:163
> +        if (alpha != configAttributeList[11])
> +            continue;
> +
> +        eglGetConfigAttrib(m_sharedDisplay, tempConfig, EGL_RED_SIZE, &red);
> +
> +        if (red != configAttributeList[3])
> +            continue;
> +
> +        eglGetConfigAttrib(m_sharedDisplay, tempConfig, EGL_GREEN_SIZE, &green);
> +
> +        if (green != configAttributeList[5])
> +            continue;
> +
> +        eglGetConfigAttrib(m_sharedDisplay, tempConfig, EGL_BLUE_SIZE, &blue);
> +
> +        if (blue != configAttributeList[7])
> +            continue;
> +
> +        eglGetConfigAttrib(m_sharedDisplay, tempConfig, EGL_SURFACE_TYPE, &surfaces);
> +
> +        if (surfaces & configAttributeList[13]) {

Please no magic indices.
You should have something like expectedAlpha, expectedGreen etc. and put them dynamically into the array.

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:172
> +    if ((m_attributes & GLPlatformSurface::SupportAlpha) && !config) {
> +        m_attributes &= ~GLPlatformSurface::SupportAlpha;
> +        config = configs[0];
> +    }

I don't quite get what the logic here is.

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:31
> +#include <glx/X11Helper.h>

EGLHelper includes something from GLX?

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:36
> +typedef X11Helper NativeWrapper;

Seems like this should be guarded with X11

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:41
> +

No need for new line

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:179
> +    m_drawable = eglCreatePixmapSurface(m_sharedDisplay, config, (EGLNativePixmapType)m_bufferHandle, 0);

No C casting please

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h:62
> +

Unnecessary line

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h:64
> +    EGLPixmapSurface(SurfaceAttributes);

explicit

[Kalyan]

Created attachment 195459 [details]
patchv3

[Kalyan]

Comment on attachment 195459 [details]
patchv3

removing request for review, will update another patch.

[Kalyan]

Created attachment 195474 [details]
patchv4

[Kalyan]

(In reply to comment #3)
> (From update of attachment 195381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195381&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:66
> >          configAttributeList[11] = m_attributes & GLPlatformSurface::SupportAlpha ? 8 : 0;
> >          configAttributeList[13] = EGL_PIXMAP_BIT;
> 
> I know this code was there before, but this is extremely bug prone. I'd prefer if instead of using a static array and changing it here with a magic index you'd just generate the array here on the fly.

Done, removed the usage of array.

> > +        if (surfaces & configAttributeList[13]) {
> 
> Please no magic indices.
> You should have something like expectedAlpha, expectedGreen etc. and put them dynamically into the array.

done.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:172
> > +    if ((m_attributes & GLPlatformSurface::SupportAlpha) && !config) {
> > +        m_attributes &= ~GLPlatformSurface::SupportAlpha;
> > +        config = configs[0];
> > +    }
> 
> I don't quite get what the logic here is.

We fallback to find a a configuration with no support for alpha channel in case we fail to find a configuration supporting alpha(when requested).  

> > Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:31
> > +#include <glx/X11Helper.h>
 
> EGLHelper includes something from GLX?

We uses nativeDisplay function from X11Helper.h class(which happens to be in glx folder) when eglNativeDisplay is called. No dependencies on GLX.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:36
> > +typedef X11Helper NativeWrapper;
> 
> Seems like this should be guarded with X11

k, I can add PLATFORM(X11) guards here

[Kalyan]

Created attachment 195636 [details]
patchv5

[Kalyan]

(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 195381 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195381&action=review
> > 
> > Seems like this should be guarded with X11
> 
> k, I can add PLATFORM(X11) guards here

Done, separated all X11 dependencies and added X11 platform guards.

[Noam Rosenthal]

Comment on attachment 195636 [details]
patchv5

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

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:100
> +    configs = new EGLConfig[numConfigs];

Nit: this can be EGLConfig configs[numConfigs]; and then you don't need to explicitly call delete.

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.h:49
> +    EGLConfig createConfig(EGLint);

EGLint expectedSurfaceType

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp:53
> +        if (success != EGL_TRUE) {
> +            LOG_ERROR("EGLInitialization failed.");
> +            m_eglDisplay = EGL_NO_DISPLAY;
> +        }

Shouldn't you return early here?

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:80
> +    if (!eglBindAPI(eglAPIVersion)) {
> +        LOG_ERROR("Failed to set EGL API(%d).", eglGetError());

Why would you need to set the API version on destruction?

[Kalyan]

Created attachment 195735 [details]
patchv6

[Kalyan]

(In reply to comment #10)
> (From update of attachment 195636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195636&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:100
> > +    configs = new EGLConfig[numConfigs];
> 
> Nit: this can be EGLConfig configs[numConfigs]; and then you don't need to explicitly call delete.
 :) changed it to be allocated on stack.

           LOG_ERROR("EGLInitialization failed.");
> > +            m_eglDisplay = EGL_NO_DISPLAY;
> > +        }
> 
> Shouldn't you return early here?

fixed.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:80
> > +    if (!eglBindAPI(eglAPIVersion)) {
> > +        LOG_ERROR("Failed to set EGL API(%d).", eglGetError());
> 
> Why would you need to set the API version on destruction?

removed.

[Kalyan]

Comment on attachment 195735 [details]
patchv6

removing request for cq, will update a new patch with changelog fixed

[Kalyan]

Created attachment 195752 [details]
patchv7

[WebKit Review Bot]

Comment on attachment 195752 [details]
patchv7

Clearing flags on attachment: 195752

Committed r147222: <http://trac.webkit.org/changeset/147222>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.