Bug 114122 - [EFL][EGL] Implement TransportSurface client.
Summary: [EFL][EGL] Implement TransportSurface client.
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: 105659
  Show dependency treegraph
 
Reported: 2013-04-07 10:16 PDT by Kalyan
Modified: 2013-05-30 05:28 PDT (History)
10 users (show)

See Also:


Attachments
patch (29.65 KB, patch)
2013-04-07 12:30 PDT, Kalyan
no flags Details | Formatted Diff | Diff
patchv2 (29.05 KB, patch)
2013-05-06 11:52 PDT, Kalyan
no flags Details | Formatted Diff | Diff
patchv3 (23.14 KB, patch)
2013-05-06 12:25 PDT, Kalyan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (594.38 KB, application/zip)
2013-05-06 14:20 PDT, Build Bot
no flags Details
patchv4 (19.13 KB, patch)
2013-05-30 02:16 PDT, Kalyan
no flags Details | Formatted Diff | Diff
patchv5 (19.14 KB, patch)
2013-05-30 03:05 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-04-07 10:16:01 PDT
This is to add support for creating texture from pixmap with EGL using GLES2
Comment 1 Kalyan 2013-04-07 12:30:18 PDT
Created attachment 196799 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2013-05-06 10:20:04 PDT
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?
Comment 3 Kalyan 2013-05-06 11:52:22 PDT
Created attachment 200732 [details]
patchv2
Comment 4 Kalyan 2013-05-06 11:54:30 PDT
(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.
Comment 5 WebKit Commit Bot 2013-05-06 11:55:02 PDT
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.
Comment 6 Kalyan 2013-05-06 11:56:07 PDT
Comment on attachment 200732 [details]
patchv2

will upload a new patch after fixing style issues
Comment 7 Kalyan 2013-05-06 12:25:28 PDT
Created attachment 200765 [details]
patchv3
Comment 8 Build Bot 2013-05-06 14:20:48 PDT
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
Comment 9 Build Bot 2013-05-06 14:20:50 PDT
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
Comment 10 Kalyan 2013-05-30 02:16:59 PDT
Created attachment 203327 [details]
patchv4
Comment 11 Kenneth Rohde Christiansen 2013-05-30 02:25:35 PDT
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
Comment 12 Kalyan 2013-05-30 02:47:40 PDT
(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.
Comment 13 Kalyan 2013-05-30 03:05:25 PDT
Created attachment 203332 [details]
patchv5
Comment 14 WebKit Commit Bot 2013-05-30 05:28:08 PDT
Comment on attachment 203332 [details]
patchv5

Clearing flags on attachment: 203332

Committed r150958: <http://trac.webkit.org/changeset/150958>
Comment 15 WebKit Commit Bot 2013-05-30 05:28:11 PDT
All reviewed patches have been landed.  Closing bug.