Summary: | [EFL][EGL] Add support for creating offscreen surface. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kalyan <kalyan.kondapally> | ||||||||||||||||
Component: | WebKit EFL | Assignee: | Kalyan <kalyan.kondapally> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cmarcelo, dino, gyuyoung.kim, kenneth, lucas.de.marchi, luiz, noam, rakuco, webkit.review.bot, zeno | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 105286, 105659 | ||||||||||||||||||
Attachments: |
|
Description
Kalyan
2013-03-26 18:06:01 PDT
Created attachment 195244 [details]
patch
Created attachment 195381 [details]
patchv2
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 Created attachment 195459 [details]
patchv3
Comment on attachment 195459 [details]
patchv3
removing request for review, will update another patch.
Created attachment 195474 [details]
patchv4
(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 Created attachment 195636 [details]
patchv5
(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 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? Created attachment 195735 [details]
patchv6
(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 on attachment 195735 [details]
patchv6
removing request for cq, will update a new patch with changelog fixed
Created attachment 195752 [details]
patchv7
Comment on attachment 195752 [details] patchv7 Clearing flags on attachment: 195752 Committed r147222: <http://trac.webkit.org/changeset/147222> All reviewed patches have been landed. Closing bug. |