Bug 113359 - [EFL][EGL] Add support for creating offscreen surface.
Summary: [EFL][EGL] Add support for creating offscreen surface.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Kalyan
URL:
Keywords:
Depends on:
Blocks: 105286 105659
  Show dependency treegraph
 
Reported: 2013-03-26 18:06 PDT by Kalyan
Modified: 2013-03-29 09:05 PDT (History)
10 users (show)

See Also:


Attachments
patch (25.43 KB, patch)
2013-03-27 01:44 PDT, Kalyan
no flags Details | Formatted Diff | Diff
patchv2 (25.57 KB, patch)
2013-03-27 13:08 PDT, Kalyan
noam: review-
Details | Formatted Diff | Diff
patchv3 (26.68 KB, patch)
2013-03-27 19:09 PDT, Kalyan
no flags Details | Formatted Diff | Diff
patchv4 (26.68 KB, patch)
2013-03-27 20:36 PDT, Kalyan
no flags Details | Formatted Diff | Diff
patchv5 (35.91 KB, patch)
2013-03-28 13:59 PDT, Kalyan
noam: review+
Details | Formatted Diff | Diff
patchv6 (35.61 KB, patch)
2013-03-29 06:35 PDT, Kalyan
no flags Details | Formatted Diff | Diff
patchv7 (35.57 KB, patch)
2013-03-29 08:22 PDT, Kalyan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kalyan 2013-03-26 18:06:01 PDT
Add support for using EGLPixmapSurface as an offscreensurface.
Comment 1 Kalyan 2013-03-27 01:44:35 PDT
Created attachment 195244 [details]
patch
Comment 2 Kalyan 2013-03-27 13:08:14 PDT
Created attachment 195381 [details]
patchv2
Comment 3 Noam Rosenthal 2013-03-27 14:47:37 PDT
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
Comment 4 Kalyan 2013-03-27 19:09:21 PDT
Created attachment 195459 [details]
patchv3
Comment 5 Kalyan 2013-03-27 19:11:20 PDT
Comment on attachment 195459 [details]
patchv3

removing request for review, will update another patch.
Comment 6 Kalyan 2013-03-27 20:36:03 PDT
Created attachment 195474 [details]
patchv4
Comment 7 Kalyan 2013-03-27 20:48:56 PDT
(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
Comment 8 Kalyan 2013-03-28 13:59:48 PDT
Created attachment 195636 [details]
patchv5
Comment 9 Kalyan 2013-03-28 14:14:24 PDT
(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.
Comment 10 Noam Rosenthal 2013-03-29 00:31:50 PDT
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?
Comment 11 Kalyan 2013-03-29 06:35:00 PDT
Created attachment 195735 [details]
patchv6
Comment 12 Kalyan 2013-03-29 06:40:55 PDT
(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.
Comment 13 Kalyan 2013-03-29 07:35:02 PDT
Comment on attachment 195735 [details]
patchv6

removing request for cq, will update a new patch with changelog fixed
Comment 14 Kalyan 2013-03-29 08:22:27 PDT
Created attachment 195752 [details]
patchv7
Comment 15 WebKit Review Bot 2013-03-29 09:05:34 PDT
Comment on attachment 195752 [details]
patchv7

Clearing flags on attachment: 195752

Committed r147222: <http://trac.webkit.org/changeset/147222>
Comment 16 WebKit Review Bot 2013-03-29 09:05:42 PDT
All reviewed patches have been landed.  Closing bug.