1682 + int render (const GLMatrix &modelview, 1683 + const GLWindowPaintAttrib &attrib); 1684 + 1685 + int render (const GLMatrix &projection, 1686 + const GLMatrix &modelview, 1687 + const GLWindowPaintAttrib &attrib); Probably for the sake of API confusion, it might be worth swapping the order of projection and modelview in this case, so you have 1685 + int render (const GLMatrix &modelview, 1686 + const GLMatrix &projection, 1687 + const GLWindowPaintAttrib &attrib); -- 1586 + GLProgram* operator () (std::list); Can that take a const std::list & ? 1116 +#if !defined(GL_BGRA) 1117 + #if !defined(GL_BGRA_EXT) 1118 + #error GL_BGRA support is required 1119 + #else 1120 + #define GL_BGRA GL_BGRA_EXT 1121 + #endif 1122 +#endif This can probably be detected in CMake by using try_compile () [1] 1703 - true 1704 + false Does this need to be off by default? 1678 + void setProgram (GLProgram *program); 1679 + 1680 + int render (const GLMatrix &modelview); 1681 + 1682 + int render (const GLMatrix &modelview, 1683 + const GLWindowPaintAttrib &attrib); 1684 + 1685 + int render (const GLMatrix &projection, 1686 + const GLMatrix &modelview, 1687 + const GLWindowPaintAttrib &attrib); The semantics of this are odd. Is GLVertexBuffer meant to be a stateful object which holds on to the state of what its rendering? In that case, it might be more appropriate to have a: void setProjection (); void setModelView (); void setAttrib (); However, on another though, perhaps all of these arguments are redundant. If glPushMatrix, glLoadMatrixf and glPopMatrix are all parts of the deprecated API, and PrivateGLVertexBuffer::render is really doing this: + GLfloat params[4] = {0, 0, 0, 0}; + GLfloat attribs[3] = {1, 1, 1}; + GLint index = 0; + + program->setUniform ("projection", projection); + program->setUniform ("modelview", modelview); + //convert paint attribs to 0-1 range + attribs[0] = attrib.opacity / 65535.0f; + attribs[1] = attrib.brightness / 65535.0f; + attribs[2] = attrib.saturation / 65535.0f; + program->setUniform3f ("paintAttrib", attribs[0], attribs[1], attribs[2]); + Then it might make more sense to have helper objects in the OpenGL plugin to handle that so that you don't need to pass matrices or paint attributes to the GLVertexBuffer at render time and we don't need to continue to expand the ::render () function argument list should we add more things to the core profile. The helper object might just server to build a GLProgram with that stuff built in so that it can be passed directly to GLVertexBuffer at render time. Just a thought. + #ifdef USE_GLES + Display *xdpy = screen->dpy (); + + glFlush (); + if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK) + { + eglSwapBuffers (eglGetDisplay (xdpy), surface); + } + else + { + #warning use proper extension for this + eglSwapBuffers (eglGetDisplay (xdpy), surface); + } + eglWaitGL (); + XFlush (xdpy); + + #else I recently merged in a change to core which uses glSwapIntervalEXT , would that work here? Also, I think that doing eglSwapBuffers here will be expensive in partial update cases, is there any reason why we can't copy from back to front buffer using glCopyPixels ? +//params is a 4-part system for determining how to draw the scene +// - x is whether or not to use a single color for drawing +// - y is whether or not to use the varying color for drawing +// - z is the number of texture units to use +// - w is the blend mode to use + +//paintAttrib is a list of opacity, brightness, and saturation values +// - x is opacity +// - y is brightness +// - z is saturation + +static std::string fragment_shader = " \n\ +#ifdef GL_ES \n\ +precision mediump float; \n\ +#endif \n\ + \n\ +uniform vec4 params; \n\ +uniform vec3 paintAttrib; \n\ It might make sense to namespace these. + #ifdef USE_GLES +// internalFormat = GL_BGRA; + internalFormat = GL_BGRA; + #else Redundant ? +int PrivateVertexBuffer::render (const GLMatrix &projection, + const GLMatrix &modelview, + const GLWindowPaintAttrib &attrib) +{ + GLfloat params[4] = {0, 0, 0, 0}; + GLfloat attribs[3] = {1, 1, 1}; + GLint index = 0; + + if (program == NULL) + { + std::cerr << "no program defined!" << std::endl; + return -1; + } You should use compLogMessage here. Also, logging this message on every render will fill the terminal with the same error message. If this is a problem, either this code should assert or it should set a flag to inhibit any further calls to render until a program is defined. + + glDeleteShader (vertex); + glDeleteShader (fragment); + + priv->valid = true; +} This isn't a problem with the code, but probably another reason to switch to using exceptions for this kind of thing. + GLint location = glGetUniformLocation (priv->program, name); + if (location == -1) + return false; This should probably warn if that fails. + #ifdef USE_GLES + Display *xdpy; + Window overlay; + EGLDisplay dpy; + EGLConfig config; + EGLint major, minor; + const char *eglExtensions, *glExtensions; + XWindowAttributes attr; + EGLint count, visualid; + EGLConfig configs[1024]; + CompOption::Vector o (0); + + const EGLint config_attribs[] = { + EGL_SURFACE_TYPE, EGL_WINDOW_BIT, + EGL_RED_SIZE, 1, + EGL_GREEN_SIZE, 1, + EGL_BLUE_SIZE, 1, + EGL_ALPHA_SIZE, 0, + EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT, + EGL_CONFIG_CAVEAT, EGL_NONE, + EGL_NONE, + }; + + const EGLint context_attribs[] = { + EGL_CONTEXT_CLIENT_VERSION, 2, + EGL_NONE + }; + + xdpy = s->dpy (); + dpy = eglGetDisplay ((EGLNativeDisplayType)xdpy); + if (!eglInitialize (dpy, &major, &minor)) .... This is very similar to the glX init code, is there any reason why this needs to be completely repeated on an #ifdef? Maybe it might make sense to break this down into stages and have a GLInitHelper base class with GLXInitHelper and EGLInitHelper. + #ifdef USE_GLES + std::map::iterator it = + boundPixmapTex.find (de->damage); + #else std::map::iterator it = boundPixmapTex.find (de->damage); + #endif If EglTexture and TfpTexture both come from the same bass class, can't that map be of GLTexture * ? + if (projection != NULL) + delete projection; + projection = new GLMatrix (projection_array); + Does priv->projection need to be a pointer ? Couldn't glScreen->projectionMatrix () return a const reference instead ? - glPushAttrib (GL_SCISSOR_BIT); - glEnable (GL_SCISSOR_TEST); glScissor (pBox->x1, screen->height () - pBox->y2, pBox->x2 - pBox->x1, pBox->y2 - pBox->y1); glClear (mask); - - glPopAttrib (); + glDisable (GL_SCISSOR_TEST); I think this will break other plugins that have set scissoring regions, no? - dlhand = dlopen (file.c_str (), RTLD_LAZY); + dlhand = dlopen (file.c_str (), RTLD_LAZY | RTLD_GLOBAL); If I remember correctly, this was about libGLESv2 breaking under RTLD_LAZY. Can you add a comment about that? The only other questions I had were why we are now disabling sync to vblank by default and why we need an invert function in GLMatrix. Other than that, looks good. I'm not an expert on OpenGL though so I might get Jay to double check this first. If those API changes aren't too difficult to make, I'd recommend making them since it means we can avoid ABI breaks in the future. [1] http://www.cmake.org/cmake/help/cmake-2-8-docs.html#command:try_compile