Bug 114122 – [EFL][EGL] Implement TransportSurface client.

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

[Kalyan]

This is to add support for creating texture from pixmap with EGL using GLES2

[Kalyan]

Created attachment 196799 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 196799 [details]
patch

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

> Source/WebCore/ChangeLog:12
> +
> +        This patch implements the client support for Transport
> +        Surface.
> +

You should say why? and what that means... like improves blah blah on X11 or so...

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

We normally use separate lines

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:200
> +    int alphaRequired = m_attributes & GLPlatformSurface::SupportAlpha ? 8 : 0;

alphaChannelsRequired?

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:51
> +    enum DisplayType {
> +        NativeDisplay = 0x00,
> +        CurrentDisplay = 0x01
> +    };

so why cant the current display be native?

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:184
> +        if (m_surface != EGL_NO_SURFACE) {
> +
> +            if (!eglBindTexImage(EGLHelper::eglDisplay(), m_surface, EGL_BACK_BUFFER))
> +                destroy();
> +        }
> +    }

why not combine?

[Kalyan]

Created attachment 200732 [details]
patchv2

[Kalyan]

(In reply to comment #2)
> (From update of attachment 196799 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196799&action=review
> > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:200
> > +    int alphaRequired = m_attributes & GLPlatformSurface::SupportAlpha ? 8 : 0;
> 
> alphaChannelsRequired?

done

> > Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:51
> > +    enum DisplayType {
> > +        NativeDisplay = 0x00,
> > +        CurrentDisplay = 0x01
> > +    };
> 
> so why cant the current display be native?

Removed its usage. It was to avoid opening new display connections.Changed it so that we check for any valid display connection before opening a new one.

[WebKit Commit Bot]

Attachment 200732 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/surfaces/efl/GLTransportSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/efl/GLTransportSurface.h', u'Source/WebCore/platform/graphics/surfaces/efl/GraphicsSurfaceCommon.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.h', u'Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h', u'Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h', u'Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.h', u'Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp', u'Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h', u'Source/WebCore/platform/graphics/surfaces/glx/X11Helper.cpp', u'Source/WebCore/platform/graphics/surfaces/glx/X11Helper.h']" exit_code: 1
Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp:130:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp:151:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:189:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:268:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:50:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kalyan]

Comment on attachment 200732 [details]
patchv2

will upload a new patch after fixing style issues

[Kalyan]

Created attachment 200765 [details]
patchv3

[Build Bot]

Comment on attachment 200765 [details]
patchv3

Attachment 200765 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/373073

New failing tests:
fast/frames/crash-remove-iframe-during-object-beforeload.html

[Build Bot]

Created attachment 200812 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Kalyan]

Created attachment 203327 [details]
patchv4

[Kenneth Rohde Christiansen]

Comment on attachment 203327 [details]
patchv4

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

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.cpp:154
> +        tempHandle =  eglCreateImageKHR(display, EGL_NO_CONTEXT, target, clientBuffer, attributes);

double space before eglCreate...

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:150
> +    if (!handle)
> +        return;
> +
> +    m_handle = handle;

why is it better to have m_handle be undefined? why not always do m_handle = handle then return after?

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:173
> +    m_totalBytes = m_size.width() * m_size.height() * 4;
> +#if USE(OPENGL_ES_2)

i would have a newline after m_totalByes =

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:175
> +    static bool  bgraSupported = GLPlatformContext::supportsGLExtension("GL_EXT_texture_format_BGRA8888");

two spaces before variable name

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:214
> +    // Fallback to use XImage in case EGLImage and TextureToPixmap are not supported.

Have you tested these code paths?

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:216
> +    m_image = XGetImage(NativeWrapper::nativeDisplay(), m_handle, 0, 0, m_size.width(), m_size.height(), AllPlanes, ZPixmap);
> +#if USE(OPENGL_ES_2)

newline after m_image =

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:221
> +#endif

newline after

[Kalyan]

(In reply to comment #11)
> (From update of attachment 203327 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203327&action=review
> 

> > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:150
> > +    if (!handle)
> > +        return;
> > +
> > +    m_handle = handle;
> 
> why is it better to have m_handle be undefined? why not always do m_handle = handle then return after?
> 

It doesn't make any difference, as the client creation should fail in the case when handle is not valid.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:214
> > +    // Fallback to use XImage in case EGLImage and TextureToPixmap are not supported.
> 
> Have you tested these code paths?

Yes, I did test them.

[Kalyan]

Created attachment 203332 [details]
patchv5

[WebKit Commit Bot]

Comment on attachment 203332 [details]
patchv5

Clearing flags on attachment: 203332

Committed r150958: <http://trac.webkit.org/changeset/150958>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.