Bug 78672

Summary:

[Texmap] Move TextureMapperGL to use GraphicsContext3D

Product:

WebKit

Reporter:

Noam Rosenthal <noam>

Component:

Layout and Rendering

Assignee:

Helder Correia <helder.correia>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adrian.yanes, dglazkov, gustavo, hausmann, hw1008.kim, joone, kevin.cs.oh, luiz, mifenton, mrobinson, ossy, philn, rtakacs, skyul, webkit.review.bot, xan.lopez, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

92441, 95037, 95069

Bug Blocks:

79766

Attachments:

Description	Flags
Patch	none
Try fix Qt4 build	none
Try fix Qt4 build	none
Fix Qt4, implement comments #8 #14	none
Part 1 - GraphicsContext3DQt	none
TextureMapperGL	none
GraphicsContext3DQt	none
GraphicsContext3DQt	none
TextureMapperGL	none
GraphicsContext3DQt	none
TextureMapperGL	none
RenderToCurrentGLContext/GC3DQt	none
texImage2DDirect	noam: review+
TextureMapperGL	noam: review-
TextureMapperGL	noam: review+, webkit-ews: commit-queue-
RenderToCurrentGLContext/GC3DQt - try fix Cr	none
Unified patch for EWS	none
patch	noam: review-, noam: commit-queue-
patch	none
patch	noam: review+, noam: commit-queue-
patch [RenderToCurrentGLContext]	none
patch [TexImage2DDirect]	none
patch [TexImage2DDirect]	webkit.review.bot: commit-queue-
patch [TexImage2DDirect]	none
patch [TextureMapperGL]	noam: commit-queue-
patch [TextureMapperGL]	none
patch	none

Noam Rosenthal

Reported 2012-02-15 00:39:23 PST

TextureMapperGL includes direct GL calls, and a lot of the glue that's already done in GraphicsContext3D. We should consoldiate it to work on top of GC3D.

Attachments
Patch (106.17 KB, patch) 2012-07-20 23:18 PDT, Helder Correia	no flags	Details Formatted Diff Diff
Try fix Qt4 build (106.31 KB, patch) 2012-07-20 23:43 PDT, Helder Correia	no flags	Details Formatted Diff Diff
Try fix Qt4 build (106.36 KB, patch) 2012-07-21 00:03 PDT, Helder Correia	no flags	Details Formatted Diff Diff
Fix Qt4, implement comments #8 #14 (108.28 KB, patch) 2012-07-21 23:38 PDT, Helder Correia	no flags	Details Formatted Diff Diff
Part 1 - GraphicsContext3DQt (10.70 KB, patch) 2012-07-23 16:44 PDT, Helder Correia	no flags	Details Formatted Diff Diff
TextureMapperGL (98.42 KB, patch) 2012-07-23 17:05 PDT, Helder Correia	no flags	Details Formatted Diff Diff
GraphicsContext3DQt (10.58 KB, patch) 2012-07-23 17:12 PDT, Helder Correia	no flags	Details Formatted Diff Diff
GraphicsContext3DQt (12.20 KB, patch) 2012-07-26 01:58 PDT, Helder Correia	no flags	Details Formatted Diff Diff
TextureMapperGL (98.47 KB, patch) 2012-07-26 02:02 PDT, Helder Correia	no flags	Details Formatted Diff Diff
GraphicsContext3DQt (13.04 KB, patch) 2012-07-26 12:55 PDT, Helder Correia	no flags	Details Formatted Diff Diff
TextureMapperGL (106.18 KB, patch) 2012-07-26 12:57 PDT, Helder Correia	no flags	Details Formatted Diff Diff
RenderToCurrentGLContext/GC3DQt (20.36 KB, patch) 2012-07-26 16:02 PDT, Helder Correia	no flags	Details Formatted Diff Diff
texImage2DDirect (5.40 KB, patch) 2012-07-26 16:13 PDT, Helder Correia	noam: review+	Details Formatted Diff Diff
TextureMapperGL (93.90 KB, patch) 2012-07-26 16:15 PDT, Helder Correia	noam: review-	Details Formatted Diff Diff
TextureMapperGL (86.31 KB, patch) 2012-07-26 16:46 PDT, Helder Correia	noam: review+ webkit-ews: commit-queue-	Details Formatted Diff Diff
RenderToCurrentGLContext/GC3DQt - try fix Cr (22.31 KB, patch) 2012-07-26 19:55 PDT, Helder Correia	no flags	Details Formatted Diff Diff
Unified patch for EWS (112.29 KB, patch) 2012-07-26 20:02 PDT, Helder Correia	no flags	Details Formatted Diff Diff
patch (114.68 KB, patch) 2012-08-12 14:57 PDT, Roland Takacs	noam: review- noam: commit-queue-	Details Formatted Diff Diff
patch (115.08 KB, patch) 2012-08-15 12:56 PDT, Roland Takacs	no flags	Details Formatted Diff Diff
patch (115.06 KB, patch) 2012-08-22 03:55 PDT, Roland Takacs	noam: review+ noam: commit-queue-	Details Formatted Diff Diff
patch [RenderToCurrentGLContext] (20.67 KB, patch) 2012-08-24 14:06 PDT, Roland Takacs	no flags	Details Formatted Diff Diff
patch [TexImage2DDirect] (4.86 KB, patch) 2012-08-24 15:08 PDT, Roland Takacs	no flags	Details Formatted Diff Diff
patch [TexImage2DDirect] (4.83 KB, patch) 2012-08-24 15:22 PDT, Roland Takacs	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
patch [TexImage2DDirect] (4.83 KB, patch) 2012-08-24 18:14 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
patch [TextureMapperGL] (81.16 KB, patch) 2012-08-25 03:24 PDT, Roland Takacs	noam: commit-queue-	Details Formatted Diff Diff
patch [TextureMapperGL] (86.28 KB, patch) 2012-08-25 12:10 PDT, Roland Takacs	no flags	Details Formatted Diff Diff
patch (86.29 KB, patch) 2012-08-28 08:26 PDT, Roland Takacs	no flags	Details Formatted Diff Diff
Show Obsolete (24) View All Add attachment proposed patch, testcase, etc.

Yael

Comment 1 2012-02-24 07:58:36 PST

Noam, Is there something specific you had in mind or is this a general observation?

Noam Rosenthal

Comment 2 2012-02-24 09:39:58 PST

(In reply to comment #1) > Noam, Is there something specific you had in mind or is this a general observation? I was thinking we should have TextureMapperGC3D, which uses the GC3D functions instead of raw GL. That's because GC3D contains a lot of glue that we could reuse instead of doing the same thing inside of TextureMapperGL.

Hyowon Kim

Comment 3 2012-02-25 21:11:53 PST

(In reply to comment #2) > (In reply to comment #1) > > Noam, Is there something specific you had in mind or is this a general observation? > I was thinking we should have TextureMapperGC3D, which uses the GC3D functions instead of raw GL. > That's because GC3D contains a lot of glue that we could reuse instead of doing the same thing inside of TextureMapperGL. I think this's a good approach~! In case of EFL port, direct GL call is not recommended, because Evas provides a structure(Evas_GL) wrapping all GL functions. I am planning to implement the support of accelerated compositing using TextureMapper in EFL port, and TextureMapper using GC3D is important to do that. Noam~ I would like to reopen bug 73111, and new patches (bug 79452, 62961) were attached for it. I was wondering if you would consider reviewing my patches about GC3D as well as Texmap.

Noam Rosenthal

Comment 4 2012-02-26 08:45:47 PST

> I think this's a good approach~! > In case of EFL port, direct GL call is not recommended, because Evas provides a structure(Evas_GL) wrapping all GL functions. > I am planning to implement the support of accelerated compositing using TextureMapper in EFL port, > and TextureMapper using GC3D is important to do that. Cool! I'd be happy to review improvements to TextureMapper. It might be good to start with creating TextureMapperGC3D alongside TextureMapperGL, because we'll need to add all the glue to Qt before we can completely replace TextureMapperGL. > > Noam~ I would like to reopen bug 73111, and new patches (bug 79452, 62961) were attached for it. > I was wondering if you would consider reviewing my patches about GC3D as well as Texmap. Sure thing! Just CC me on bugs you'd like me to review and I'll try when I can. If I'm not comfortable reviewing something I'd let you know.

Helder Correia

Comment 5 2012-07-20 23:18:00 PDT

Created attachment 153650 [details] Patch

Helder Correia

Comment 6 2012-07-20 23:21:01 PDT

@Martin Robinson: I tried to build the GTK port myself and figure out whether this breaks the build and if I could make any necessary changes to adapt to this patch myself. Unfortunately, I had no luck. Could you please have a look at and let me know the answers? Appreciate.

Early Warning System Bot

Comment 7 2012-07-20 23:22:43 PDT

Comment on attachment 153650 [details] Patch Attachment 153650 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13311530

Martin Robinson

Comment 8 2012-07-20 23:23:38 PDT

Comment on attachment 153650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153650&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:48 > + precision mediump float; > uniform sampler2D s_source; > uniform sampler2D s_mask; > - uniform lowp float u_opacity; > - varying highp vec2 v_sourceTexCoord; > - varying highp vec2 v_maskTexCoord; > + uniform float u_opacity; > + varying vec2 v_sourceTexCoord; > + varying vec2 v_maskTexCoord; > void main(void) > { > - lowp vec4 color = texture2D(s_source, v_sourceTexCoord); > - lowp vec4 maskColor = texture2D(s_mask, v_maskTexCoord); > - lowp float fragmentAlpha = u_opacity * maskColor.a; > + vec4 color = texture2D(s_source, v_sourceTexCoord); > + vec4 maskColor = texture2D(s_mask, v_maskTexCoord); > + float fragmentAlpha = u_opacity * maskColor.a; > gl_FragColor = vec4(color.rgb * fragmentAlpha, color.a * fragmentAlpha); Perhaps it makes sense to change the precision of the uniforms and varyings in another patch?

Helder Correia

Comment 9 2012-07-20 23:43:28 PDT

Created attachment 153653 [details] Try fix Qt4 build

Early Warning System Bot

Comment 10 2012-07-20 23:50:29 PDT

Comment on attachment 153653 [details] Try fix Qt4 build Attachment 153653 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13304543

Helder Correia

Comment 11 2012-07-21 00:03:05 PDT

Created attachment 153657 [details] Try fix Qt4 build

Early Warning System Bot

Comment 12 2012-07-21 00:06:42 PDT

Comment on attachment 153657 [details] Try fix Qt4 build Attachment 153657 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13308521

Noam Rosenthal

Comment 13 2012-07-21 08:48:57 PDT

(In reply to comment #8) > (From update of attachment 153650 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153650&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:48 > > + precision mediump float; > > uniform sampler2D s_source; > > uniform sampler2D s_mask; > > - uniform lowp float u_opacity; > > - varying highp vec2 v_sourceTexCoord; > > - varying highp vec2 v_maskTexCoord; > > + uniform float u_opacity; > > + varying vec2 v_sourceTexCoord; > > + varying vec2 v_maskTexCoord; > > void main(void) > > { > > - lowp vec4 color = texture2D(s_source, v_sourceTexCoord); > > - lowp vec4 maskColor = texture2D(s_mask, v_maskTexCoord); > > - lowp float fragmentAlpha = u_opacity * maskColor.a; > > + vec4 color = texture2D(s_source, v_sourceTexCoord); > > + vec4 maskColor = texture2D(s_mask, v_maskTexCoord); > > + float fragmentAlpha = u_opacity * maskColor.a; > > gl_FragColor = vec4(color.rgb * fragmentAlpha, color.a * fragmentAlpha); > > Perhaps it makes sense to change the precision of the uniforms and varyings in another patch? Yes, good idea.

Noam Rosenthal

Comment 14 2012-07-21 09:23:58 PDT

Comment on attachment 153657 [details] Try fix Qt4 build View in context: https://bugs.webkit.org/attachment.cgi?id=153657&action=review Thanks for working on this laborious patch! See comments. > Source/WebCore/ChangeLog:10 > + TextureMapperGL (TMGL) includes direct GL calls and > + GraphicsContext3D (GC3D) offers many conveniences over the > + former approach. This patch changes the backend of TMGL to For example, using existing CSS shader code, ANGLE for shader compilation, reusing WebCore::Texture, having shaders and textures that can delete themselves. > Source/WebCore/ChangeLog:13 > + TextureMapperGC3D), but in the end it was agreed to consolidate > + it inside TMGL instead. The rationale is that an additional TextureMapper subclass would get stale very fast. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:127 > + // In this case, we are not setting up WebGL, but rather TextureMapperGL, and want to rely We might use it for other things than TextureMapperGL. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:376 > + // In this case, we are not setting up WebGL, but rather TextureMapperGL, hence skip all > + // canvas and ANGLE logic. We shouldn't skip the ANGLE logic. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:43 > +#if OS(DARWIN) I think we can move all the OS(DARWIN) stuff under !USE(TEXMAP_OPENGL_ES_2), apart from defining TRANSFER_TYPE to INT_8_8_8... > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:52 > +#if !defined(TEXMAP_OPENGL_ES_2) > +#define GL_UNPACK_ROW_LENGTH 0x0CF2 > +#define GL_UNPACK_SKIP_PIXELS 0x0CF4 > +#define GL_UNPACK_SKIP_ROWS 0x0CF3 > #endif Add a FIXME: Move this to Extensions3D > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:200 > + m_context3D = GraphicsContext3D::create(attributes, 0, GraphicsContext3D::RenderDirectlyToHostWindow); Add a /* ... */ comment next to the magical 0 > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:201 > + m_data = new TextureMapperGLData(this); You don't really use anything from TextureMapper other than the gc3d... why don't we just pass that as a parameter? > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:287 > + const GC3Dfloat m4[] = { > matrix.m11(), matrix.m12(), matrix.m13(), matrix.m14(), > matrix.m21(), matrix.m22(), matrix.m23(), matrix.m24(), > matrix.m31(), matrix.m32(), matrix.m33(), matrix.m34(), > matrix.m41(), matrix.m42(), matrix.m43(), matrix.m44() > }; > - GL_CMD(glUniformMatrix4fv(shaderProgram->matrixLocation(), 1, GL_FALSE, m4)); > + m_context3D->uniformMatrix4fv(shaderProgram->matrixLocation(), 1, false, const_cast<GC3Dfloat*>(m4)); Instead of const_cast, why don't you make the array non-const to beginw ith?

Helder Correia

Comment 15 2012-07-21 23:38:27 PDT

Created attachment 153684 [details] Fix Qt4, implement comments #8 #14

Noam Rosenthal

Comment 16 2012-07-22 07:47:05 PDT

Comment on attachment 153684 [details] Fix Qt4, implement comments #8 #14 View in context: https://bugs.webkit.org/attachment.cgi?id=153684&action=review I'm OK with this, but it probably needs some change to GC3DCairo. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:520 > + fragmentShaderSource = STRINGIFY( > varying highp vec2 v_texCoord; precision mediump float. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:553 > + fragmentShaderSource = STRINGIFY( > varying highp vec2 v_texCoord; precision mediump float.

Helder Correia

Comment 17 2012-07-23 16:44:20 PDT

Created attachment 153896 [details] Part 1 - GraphicsContext3DQt

Helder Correia

Comment 18 2012-07-23 17:05:28 PDT

Created attachment 153906 [details] TextureMapperGL

Helder Correia

Comment 19 2012-07-23 17:12:29 PDT

Created attachment 153909 [details] GraphicsContext3DQt

Simon Hausmann

Comment 20 2012-07-24 00:19:05 PDT

Comment on attachment 153909 [details] GraphicsContext3DQt View in context: https://bugs.webkit.org/attachment.cgi?id=153909&action=review It sounds to me that this isn't actually RenderDirectToHostWindow, because this isn't using the HostWindow's surface or any other property of it. It sounds to me that this is a new RenderStyle, something along the lines of RenderToCurrentContext. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:381 > + RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs, hostWindow, renderDirectlyToHostWindow)); Wouldn't it be cleaner to write renderStyle == RenderDirectlyToHostWindow instead of implicitly converting GC3D::RenderStyle to bool?

Noam Rosenthal

Comment 21 2012-07-24 05:10:21 PDT

(In reply to comment #20) > (From update of attachment 153909 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153909&action=review > > It sounds to me that this isn't actually RenderDirectToHostWindow, because this isn't using the HostWindow's surface or any other property of it. It sounds to me that this is a new RenderStyle, something along the lines of RenderToCurrentContext. > > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:381 > > + RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs, hostWindow, renderDirectlyToHostWindow)); > > Wouldn't it be cleaner to write renderStyle == RenderDirectlyToHostWindow instead of implicitly converting GC3D::RenderStyle to bool? I argee, we should add another RenderStyle value.

Noam Rosenthal

Comment 22 2012-07-24 05:52:09 PDT

(In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 153909 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=153909&action=review > > > > It sounds to me that this isn't actually RenderDirectToHostWindow, because this isn't using the HostWindow's surface or any other property of it. It sounds to me that this is a new RenderStyle, something along the lines of RenderToCurrentContext. > > > > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:381 > > > + RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs, hostWindow, renderDirectlyToHostWindow)); > > > > Wouldn't it be cleaner to write renderStyle == RenderDirectlyToHostWindow instead of implicitly converting GC3D::RenderStyle to bool? > > I argee, we should add another RenderStyle value. actually, maybe it would be more explicit to have a new function, static GraphicsContext3D::createWrapperForCurrentGLContext() Since using the existing function with parameters we don't use (attributes, hostwindow) can be confusing. Also, when we are a wrapper to the current GL context, makeContextCurrent should not bind FBO 0.

Simon Hausmann

Comment 23 2012-07-24 07:55:39 PDT

(In reply to comment #22) > actually, maybe it would be more explicit to have a new function, > static GraphicsContext3D::createWrapperForCurrentGLContext() > > Since using the existing function with parameters we don't use (attributes, hostwindow) can be confusing. > > Also, when we are a wrapper to the current GL context, makeContextCurrent should not bind FBO 0. That's a great idea :)

Helder Correia

Comment 24 2012-07-26 01:58:25 PDT

Created attachment 154580 [details] GraphicsContext3DQt

Helder Correia

Comment 25 2012-07-26 02:02:03 PDT

Created attachment 154582 [details] TextureMapperGL

Early Warning System Bot

Comment 26 2012-07-26 02:41:28 PDT

Comment on attachment 154582 [details] TextureMapperGL Attachment 154582 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13347689

Early Warning System Bot

Comment 27 2012-07-26 03:37:34 PDT

Comment on attachment 154582 [details] TextureMapperGL Attachment 154582 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13347705

Simon Hausmann

Comment 28 2012-07-26 07:51:17 PDT

Comment on attachment 154580 [details] GraphicsContext3DQt View in context: https://bugs.webkit.org/attachment.cgi?id=154580&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:871 > + GraphicsContext3D(Attributes, HostWindow*, RenderStyle = RenderOffscreen); > GraphicsContext3D(Attributes attrs, HostWindow* hostWindow, bool renderDirectlyToHostWindow); So the first constructor is implemented in GraphicsContext3DQt.cpp and the second constructor is implemented by all the other ports. Wouldn't it be nicer to have only one constructor? > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:363 > +void GraphicsContext3DPrivate::initializeANGLE() > +{ > + ShBuiltInResources ANGLEResources; > + ShInitBuiltInResources(&ANGLEResources); > + > + m_context->getIntegerv(GraphicsContext3D::MAX_VERTEX_ATTRIBS, &ANGLEResources.MaxVertexAttribs); > + m_context->getIntegerv(GraphicsContext3D::MAX_VERTEX_UNIFORM_VECTORS, &ANGLEResources.MaxVertexUniformVectors); > + m_context->getIntegerv(GraphicsContext3D::MAX_VARYING_VECTORS, &ANGLEResources.MaxVaryingVectors); > + m_context->getIntegerv(GraphicsContext3D::MAX_VERTEX_TEXTURE_IMAGE_UNITS, &ANGLEResources.MaxVertexTextureImageUnits); > + m_context->getIntegerv(GraphicsContext3D::MAX_COMBINED_TEXTURE_IMAGE_UNITS, &ANGLEResources.MaxCombinedTextureImageUnits); > + m_context->getIntegerv(GraphicsContext3D::MAX_TEXTURE_IMAGE_UNITS, &ANGLEResources.MaxTextureImageUnits); > + m_context->getIntegerv(GraphicsContext3D::MAX_FRAGMENT_UNIFORM_VECTORS, &ANGLEResources.MaxFragmentUniformVectors); > + > + // Always set to 1 for OpenGL ES. > + ANGLEResources.MaxDrawBuffers = 1; > + m_context->m_compiler.setResources(ANGLEResources); > +} I wonder why this code isn't in some file shared between the ports? (I know, that's not part of your patch, just generally wondering :) > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:391 > +PassRefPtr<GraphicsContext3D> GraphicsContext3D::createWrapperForCurrentGLContext() > +{ > + RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(Attributes(), 0, GraphicsContext3D::RenderToCurrentGLContext)); > return context->m_private ? context.release() : 0; > } Doesn't this function belong into GraphicsContext3D.cpp instead of GraphicsContext3DQt.cpp?

Gustavo Noronha (kov)

Comment 29 2012-07-26 08:08:42 PDT

Comment on attachment 154582 [details] TextureMapperGL Attachment 154582 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13347809

Helder Correia

Comment 30 2012-07-26 08:47:31 PDT

(In reply to comment #28) > (From update of attachment 154580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154580&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:871 > > + GraphicsContext3D(Attributes, HostWindow*, RenderStyle = RenderOffscreen); > > GraphicsContext3D(Attributes attrs, HostWindow* hostWindow, bool renderDirectlyToHostWindow); > > So the first constructor is implemented in GraphicsContext3DQt.cpp and the second constructor is implemented by all the other ports. Wouldn't it be nicer to have only one constructor? Yes, and that's what I intended to do, but as I'd have to touch a handful of files from all other ports (and make sure I don't brake it), I'd prefer not to include yet another refactoring in this patch. > > + > > + // Always set to 1 for OpenGL ES. > > + ANGLEResources.MaxDrawBuffers = 1; > > + m_context->m_compiler.setResources(ANGLEResources); > > +} > > I wonder why this code isn't in some file shared between the ports? (I know, that's not part of your patch, just generally wondering :) There is plenty of code in these files that could be shared, waiting for someone to consolidate it (in other patches, as I you said :) I can help with that myself at some point. > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:391 > > +PassRefPtr<GraphicsContext3D> GraphicsContext3D::createWrapperForCurrentGLContext() > > +{ > > + RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(Attributes(), 0, GraphicsContext3D::RenderToCurrentGLContext)); > > return context->m_private ? context.release() : 0; > > } > > Doesn't this function belong into GraphicsContext3D.cpp instead of GraphicsContext3DQt.cpp? Indeed, copy/paste lapse.

Helder Correia

Comment 31 2012-07-26 09:16:45 PDT

> > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:391 > > > +PassRefPtr<GraphicsContext3D> GraphicsContext3D::createWrapperForCurrentGLContext() > > > +{ > > > + RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(Attributes(), 0, GraphicsContext3D::RenderToCurrentGLContext)); > > > return context->m_private ? context.release() : 0; > > > } > > > > Doesn't this function belong into GraphicsContext3D.cpp instead of GraphicsContext3DQt.cpp? > > Indeed, copy/paste lapse. Actually, it was on purpose, so that the other builds are not broken because of the call to the new constructor they don't implement. What should I do, go for the refactoring I was trying to avoid in the first place and touch all ports, making them use the new constructor? Wrap with an #if? Leave it like this for this patch?

Noam Rosenthal

Comment 32 2012-07-26 09:38:12 PDT

(In reply to comment #31) > > > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:391 > > > > +PassRefPtr<GraphicsContext3D> GraphicsContext3D::createWrapperForCurrentGLContext() > > > > +{ > > > > + RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(Attributes(), 0, GraphicsContext3D::RenderToCurrentGLContext)); > > > > return context->m_private ? context.release() : 0; > > > > } > > > > > > Doesn't this function belong into GraphicsContext3D.cpp instead of GraphicsContext3DQt.cpp? > > > > Indeed, copy/paste lapse. > > Actually, it was on purpose, so that the other builds are not broken because of the call to the new constructor they don't implement. What should I do, go for the refactoring I was trying to avoid in the first place and touch all ports, making them use the new constructor? Wrap with an #if? Leave it like this for this patch? It actually belongs in GraphicsContextGLCommon, since ports that don't use standard GL wouldn't have a "current GL context". i think the best way to go is to change all those constructors from bool to RenderStyle. it doesn't sound like a huge refactor.

Helder Correia

Comment 33 2012-07-26 12:55:56 PDT

Created attachment 154725 [details] GraphicsContext3DQt

Helder Correia

Comment 34 2012-07-26 12:57:50 PDT

Created attachment 154727 [details] TextureMapperGL

Helder Correia

Comment 35 2012-07-26 16:02:00 PDT

Created attachment 154777 [details] RenderToCurrentGLContext/GC3DQt

Helder Correia

Comment 36 2012-07-26 16:13:17 PDT

Created attachment 154782 [details] texImage2DDirect

Helder Correia

Comment 37 2012-07-26 16:15:14 PDT

Created attachment 154783 [details] TextureMapperGL

Noam Rosenthal

Comment 38 2012-07-26 16:25:03 PDT

Comment on attachment 154783 [details] TextureMapperGL View in context: https://bugs.webkit.org/attachment.cgi?id=154783&action=review removing the precision identifiers is nice, but should go in a future patch (we don't even have to do it at all). > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:196 > + GraphicsContext3D::Attributes attributes; > + attributes.antialias = false; > + attributes.depth = false; I don't think you need these lines > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:408 > switch (type) { > case FilterOperation::GRAYSCALE: > fragmentShaderSource = STANDARD_FILTER( > - lowp vec4 shade(lowp vec4 color) > + vec4 shade(vec4 color) > { > - lowp float amount = 1.0 - u_amount; > + float amount = 1.0 - u_amount; > return vec4((0.2126 + 0.7874 * amount) * color.r + (0.7152 - 0.7152 * amount) * color.g + (0.0722 - 0.0722 * amount) * color.b, > (0.2126 - 0.2126 * amount) * color.r + (0.7152 + 0.2848 * amount) * color.g + (0.0722 - 0.0722 * amount) * color.b, > (0.2126 - 0.2126 * amount) * color.r + (0.7152 - 0.7152 * amount) * color.g + (0.0722 + 0.9278 * amount) * color.b, I think we decided in a previous comment to leave the removal of the precision identifiers to a separate patch.

Helder Correia

Comment 39 2012-07-26 16:46:39 PDT

Created attachment 154787 [details] TextureMapperGL

Noam Rosenthal

Comment 40 2012-07-26 16:51:09 PDT

Comment on attachment 154787 [details] TextureMapperGL Great! We have to wait with landing until we implement the new RenderStyle for GraphicsContext3DCairo.

WebKit Review Bot

Comment 41 2012-07-26 17:46:53 PDT

Comment on attachment 154777 [details] RenderToCurrentGLContext/GC3DQt Attachment 154777 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13372094

Helder Correia

Comment 42 2012-07-26 19:55:42 PDT

Created attachment 154818 [details] RenderToCurrentGLContext/GC3DQt - try fix Cr

Helder Correia

Comment 43 2012-07-26 20:02:49 PDT

Created attachment 154819 [details] Unified patch for EWS

Early Warning System Bot

Comment 44 2012-07-26 23:06:45 PDT

Comment on attachment 154787 [details] TextureMapperGL Attachment 154787 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13373192

Early Warning System Bot

Comment 45 2012-07-26 23:44:47 PDT

Comment on attachment 154787 [details] TextureMapperGL Attachment 154787 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13368261

WebKit Review Bot

Comment 46 2012-07-27 05:01:33 PDT

Comment on attachment 154777 [details] RenderToCurrentGLContext/GC3DQt Cleared Noam Rosenthal's review+ from obsolete attachment 154777 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Roland Takacs

Comment 47 2012-08-12 14:57:01 PDT

Created attachment 157911 [details] patch Must not to call "makeContextCurrent()" when we would like to use functions that wrapped by GraphicsContext3D. Else it does not draw. > bool GraphicsContext3D::makeContextCurrent() > { > if (!m_private || m_renderStyle == RenderToCurrentGLContext) > return false; > > return m_private->makeCurrentIfNeeded(); > } m_renderStyle indicates we want to call GC3D functions within WebKit code (like TextureMapper). When there is no hostWindow (e.g. renderStyle is RenderToCurrentGLContext) must not to release m_surface at the ~GraphicsContext3DPrivate. > if (renderStyle == GraphicsContext3D::RenderToCurrentGLContext) { > m_platformContext = QOpenGLContext::currentContext(); << QUERY > m_surface = m_platformContext->surface(); << QUERY > return; > } You can see if there is not hostWindow m_platformContext and m_surface are not created only queried. So I inserted a check mechanism to eliminat unwanted release. Else there is a nice sefault :) > GraphicsContext3DPrivate::~GraphicsContext3DPrivate() > { > if (m_hostWindow) { > delete m_surface; > m_surface = 0; > // Platform context is assumed to be owned by surface. > m_platformContext = 0; > } > } Who created m_platformContext and m_surface will destroy them. Finally I added a new constant to GraphicsContext3D macros that is BGRA. When I updated the patch for now I found GL_BGRA within TextureMapperGL.cpp. Because it was not defined I defined it :)

Kenneth Rohde Christiansen

Comment 48 2012-08-13 11:11:05 PDT

Comment on attachment 157911 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=157911&action=review > Source/WebCore/ChangeLog:10 > + TextureMapperGL (TMGL) includes direct GL calls and > + GraphicsContext3D (GC3D) offers many conveniences over the > + former approach: using existing CSS shader code, ANGLE for > + shader compilation, reusing WebCore::Texture, having shaders and > + textures that can delete themselves. Maybe better to list these in list form? - ANGLE for shader compilation - Reusing WebCore::Texture... > Source/WebCore/ChangeLog:16 > + This patch changes the backend of TMGL to GC3D. It was initially > + thought to introduce a new backend (say, TextureMapperGC3D), but > + in the end it was agreed to consolidate it inside TMGL instead, > + as an additional TextureMapper subclass would get stale very > + quickly. Is this history important? > Source/WebCore/ChangeLog:169 > + > +2012-08-12 Helder Correia <helder.correia@nokia.com> > + > + [Texmap] Move TextureMapperGL to use GraphicsContext3D > + https://bugs.webkit.org/show_bug.cgi?id=78672 > + > + Reviewed by NOBODY (OOPS!). Why does this include other changelog entries? > Source/WebCore/platform/graphics/GraphicsContext3D.h:482 > + static PassRefPtr<GraphicsContext3D> createWrapperForCurrentGLContext(); I would remove Wrapper from the name. createForCurrentGLContext() seems descriptive enough for me given the class name > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:214 > + delete m_surface; > + m_surface = 0; OwnPtr + clear() ?

Noam Rosenthal

Comment 49 2012-08-13 11:36:48 PDT

Comment on attachment 157911 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=157911&action=review I have no issues with the actual patch - it's beginning to look real good. See Kenneth's comments though :) >> Source/WebCore/ChangeLog:169 >> + Reviewed by NOBODY (OOPS!). > > Why does this include other changelog entries? I think it would be best to squash these to one changelog entry, and to say that it's based on a previous patch by Helder Correia. >> Source/WebCore/platform/graphics/GraphicsContext3D.h:482 >> + static PassRefPtr<GraphicsContext3D> createWrapperForCurrentGLContext(); > > I would remove Wrapper from the name. createForCurrentGLContext() seems descriptive enough for me given the class name Either is OK with me, let's go with what Kenneth is suggesting :)

Roland Takacs

Comment 50 2012-08-15 12:56:30 PDT

Created attachment 158621 [details] patch

Kenneth Rohde Christiansen

Comment 51 2012-08-15 13:57:47 PDT

Comment on attachment 158621 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=158621&action=review > Source/WebCore/ChangeLog:1 > +2012-08-12 Helder Correia <helder.correia@nokia.com> Shouldnt this be your name?

Roland Takacs

Comment 52 2012-08-15 14:35:05 PDT

(In reply to comment #51) > (From update of attachment 158621 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158621&action=review > > > Source/WebCore/ChangeLog:1 > > +2012-08-12 Helder Correia <helder.correia@nokia.com> > > Shouldnt this be your name? I don't think so. This patch was created by Helder. All I had to do is find and fix why did not draw webkit the webgl contents. It is true, I found another issue with this patch in the meantime (segfault when refresh), but the 99% of patch is written by Helder, my 1% is only searching and resolving these issues. btw I inserted the following row into changelog: The patch was originally developed by Helder Correia and finished by Roland Takacs (rtakacs@inf.u-szeged.hu)

Roland Takacs

Comment 53 2012-08-22 03:55:12 PDT

Created attachment 159896 [details] patch Tested on GTK. It works!

Noam Rosenthal

Comment 54 2012-08-22 07:19:55 PDT

(In reply to comment #53) > Created an attachment (id=159896) [details] > patch > > Tested on GTK. It works! Does the leaves demo show the leaves?

Roland Takacs

Comment 55 2012-08-22 08:22:37 PDT

(In reply to comment #54) > (In reply to comment #53) > > Created an attachment (id=159896) [details] [details] > > patch > > > > Tested on GTK. It works! > > Does the leaves demo show the leaves? I don't know what is the leaves demo I checked only the following pages: * https://www.khronos.org/registry/webgl/sdk/demos/google/particles/index.html * http://webglsamples.googlecode.com/hg/aquarium/aquarium.html * http://alteredqualia.com/three/examples/webgl_pasta.html * http://learningwebgl.com/lessons/lesson01/index.html * http://learningwebgl.com/lessons/lesson09/index.html * http://www.spielzeugz.de/html5/sticky-thing/ * http://helloracer.com/webgl/ These appear good :)

Simon Hausmann

Comment 56 2012-08-23 00:25:07 PDT

(In reply to comment #55) > (In reply to comment #54) > > (In reply to comment #53) > > > Created an attachment (id=159896) [details] [details] [details] > > > patch > > > > > > Tested on GTK. It works! > > > > Does the leaves demo show the leaves? > > I don't know what is the leaves demo It's an accelerated compositing classic :) http://www.webkit.org/blog-files/leaves/

Roland Takacs

Comment 57 2012-08-23 02:16:12 PDT

(In reply to comment #56) > (In reply to comment #55) > > (In reply to comment #54) > > > (In reply to comment #53) > > > > Created an attachment (id=159896) [details] [details] [details] [details] > > > > patch > > > > > > > > Tested on GTK. It works! > > > > > > Does the leaves demo show the leaves? > > > > I don't know what is the leaves demo > > It's an accelerated compositing classic :) > > http://www.webkit.org/blog-files/leaves/ :) thx I checked this page with GTKLauncher and the leaves were falling down :) [of course with this patch]

Noam Rosenthal

Comment 58 2012-08-23 09:39:17 PDT

Comment on attachment 159896 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=159896&action=review Please land the GC3D changes first, to make sure they don't break anything (I already reviewed them earlier) And then we can land the TM changes. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.h:225 > + TextureMapperShaderManager() { }; Extra ;

Roland Takacs

Comment 59 2012-08-24 14:06:18 PDT

Created attachment 160499 [details] patch [RenderToCurrentGLContext] part 1 - I did not find issues with this patch so it works.

Roland Takacs

Comment 60 2012-08-24 15:08:39 PDT

Created attachment 160508 [details] patch [TexImage2DDirect]

Roland Takacs

Comment 61 2012-08-24 15:22:58 PDT

Created attachment 160511 [details] patch [TexImage2DDirect]

WebKit Review Bot

Comment 62 2012-08-24 15:29:54 PDT

Comment on attachment 160499 [details] patch [RenderToCurrentGLContext] Clearing flags on attachment: 160499 Committed r126635: <http://trac.webkit.org/changeset/126635>

WebKit Review Bot

Comment 63 2012-08-24 15:30:03 PDT

All reviewed patches have been landed. Closing bug.

Noam Rosenthal

Comment 64 2012-08-24 18:04:03 PDT

Still missing some patches.

WebKit Review Bot

Comment 65 2012-08-24 18:11:40 PDT

Comment on attachment 160511 [details] patch [TexImage2DDirect] Rejecting attachment 160511 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 No'am Rosenthal found in /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/13592388

Noam Rosenthal

Comment 66 2012-08-24 18:14:19 PDT

Created attachment 160535 [details] patch [TexImage2DDirect]

WebKit Review Bot

Comment 67 2012-08-24 19:25:44 PDT

Comment on attachment 160535 [details] patch [TexImage2DDirect] Clearing flags on attachment: 160535 Committed r126675: <http://trac.webkit.org/changeset/126675>

WebKit Review Bot

Comment 68 2012-08-24 19:25:52 PDT

All reviewed patches have been landed. Closing bug.

Roland Takacs

Comment 69 2012-08-25 03:24:54 PDT

Created attachment 160559 [details] patch [TextureMapperGL] final part of the big patch.

Noam Rosenthal

Comment 70 2012-08-25 09:27:17 PDT

Last patch to go

Noam Rosenthal

Comment 71 2012-08-25 11:49:22 PDT

Comment on attachment 160559 [details] patch [TextureMapperGL] View in context: https://bugs.webkit.org/attachment.cgi?id=160559&action=review > Source/WebCore/ChangeLog:20 > + This patch changes the backend of TMGL to GC3D. It was initially > + thought to introduce a new backend (say, TextureMapperGC3D), but > + in the end it was agreed to consolidate it inside TMGL instead, > + as an additional TextureMapper subclass would get stale very > + quickly. Please remove this from the Changelog.

Roland Takacs

Comment 72 2012-08-25 12:10:48 PDT

Created attachment 160567 [details] patch [TextureMapperGL]

WebKit Review Bot

Comment 73 2012-08-25 18:51:03 PDT

Comment on attachment 160567 [details] patch [TextureMapperGL] Clearing flags on attachment: 160567 Committed r126694: <http://trac.webkit.org/changeset/126694>

WebKit Review Bot

Comment 74 2012-08-25 18:51:12 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 75 2012-08-26 12:41:59 PDT

(In reply to comment #73) > (From update of attachment 160567 [details]) > Clearing flags on attachment: 160567 > > Committed r126694: <http://trac.webkit.org/changeset/126694> It broke the debug build - https://bugs.webkit.org/show_bug.cgi?id=95037

WebKit Review Bot

Comment 76 2012-08-27 03:45:20 PDT

Re-opened since this is blocked by 95069

Roland Takacs

Comment 77 2012-08-28 08:26:17 PDT

Created attachment 160989 [details] patch fixed patch

Csaba Osztrogonác

Comment 78 2012-08-28 08:54:38 PDT

(In reply to comment #77) > Created an attachment (id=160989) [details] > patch > > fixed patch Could you describe what was the problem with the previous patch, please?

Csaba Osztrogonác

Comment 79 2012-08-28 09:13:35 PDT

(In reply to comment #78) > (In reply to comment #77) > > Created an attachment (id=160989) [details] [details] > > patch > > > > fixed patch > > Could you describe what was the problem with the previous patch, please? Here is the diff (---: rolled out patch +++: new patch): ------------------------------------------------------------ --- a/Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp +++ b/Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp -+ context3D->texImage2DDirect(GraphicsContext3D::TEXTURE_2D, 0, format, m_textureSize.width(), m_textureSize.height(), 0, format, DEFAULT_TEXTURE_PIXEL_TRANSFER_TYPE, 0); ++ context3D->texImage2DDirect(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, m_textureSize.width(), m_textureSize.height(), 0, format, DEFAULT_TEXTURE_PIXEL_TRANSFER_TYPE, 0);

Roland Takacs

Comment 80 2012-08-28 09:43:26 PDT

(In reply to comment #79) > (In reply to comment #78) > > (In reply to comment #77) > > > Created an attachment (id=160989) [details] [details] [details] > > > patch > > > > > > fixed patch > > > > Could you describe what was the problem with the previous patch, please? > > Here is the diff (---: rolled out patch +++: new patch): > ------------------------------------------------------------ > --- a/Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp > +++ b/Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp > -+ context3D->texImage2DDirect(GraphicsContext3D::TEXTURE_2D, 0, format, m_textureSize.width(), m_textureSize.height(), 0, format, DEFAULT_TEXTURE_PIXEL_TRANSFER_TYPE, 0); > ++ context3D->texImage2DDirect(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, m_textureSize.width(), m_textureSize.height(), 0, format, DEFAULT_TEXTURE_PIXEL_TRANSFER_TYPE, 0); sure, this change resolves the problem that happened on wk2 :)

Martin Robinson

Comment 81 2012-08-28 23:54:42 PDT

Comment on attachment 160989 [details] patch This patch leaves GTK+ accelerated compositing in a broken state, since Cairo GraphicsContext3D does not support conjuring a context from the current context. I'll try to add this to the patch tomorrow.

Martin Robinson

Comment 82 2012-08-29 13:18:59 PDT

Comment on attachment 160989 [details] patch With the patch posted at bug 92441, this patch works again. I'm restoring the review flag, though this should probably land after 92441 is closed.

Noam Rosenthal

Comment 83 2012-08-29 13:47:02 PDT

Comment on attachment 160989 [details] patch It's already r+ed, waiting for the other patch to land before landing this one.

WebKit Review Bot

Comment 84 2012-08-29 16:12:22 PDT

Comment on attachment 160989 [details] patch Clearing flags on attachment: 160989 Committed r127063: <http://trac.webkit.org/changeset/127063>

WebKit Review Bot

Comment 85 2012-08-29 16:12:33 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 86 2012-08-29 21:34:20 PDT

(In reply to comment #84) > (From update of attachment 160989 [details]) > Clearing flags on attachment: 160989 > > Committed r127063: <http://trac.webkit.org/changeset/127063> And I landed Simon's debug buildfix again - https://trac.webkit.org/changeset/127096 (It should have been integrated to the 2nd patch.)

Note You need to log in before you can comment on or make changes to this bug.