Bug 78672

Summary: [Texmap] Move TextureMapperGL to use GraphicsContext3D
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: Layout and RenderingAssignee: 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: P2    
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

Description Noam Rosenthal 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.
Comment 1 Yael 2012-02-24 07:58:36 PST
Noam, Is there something specific you had in mind or is this a general observation?
Comment 2 Noam Rosenthal 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.
Comment 3 Hyowon Kim 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.
Comment 4 Noam Rosenthal 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.
Comment 5 Helder Correia 2012-07-20 23:18:00 PDT
Created attachment 153650 [details]
Patch
Comment 6 Helder Correia 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.
Comment 7 Early Warning System Bot 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
Comment 8 Martin Robinson 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?
Comment 9 Helder Correia 2012-07-20 23:43:28 PDT
Created attachment 153653 [details]
Try fix Qt4 build
Comment 10 Early Warning System Bot 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
Comment 11 Helder Correia 2012-07-21 00:03:05 PDT
Created attachment 153657 [details]
Try fix Qt4 build
Comment 12 Early Warning System Bot 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
Comment 13 Noam Rosenthal 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.
Comment 14 Noam Rosenthal 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?
Comment 15 Helder Correia 2012-07-21 23:38:27 PDT
Created attachment 153684 [details]
Fix Qt4, implement comments #8 #14
Comment 16 Noam Rosenthal 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.
Comment 17 Helder Correia 2012-07-23 16:44:20 PDT
Created attachment 153896 [details]
Part 1 - GraphicsContext3DQt
Comment 18 Helder Correia 2012-07-23 17:05:28 PDT
Created attachment 153906 [details]
TextureMapperGL
Comment 19 Helder Correia 2012-07-23 17:12:29 PDT
Created attachment 153909 [details]
GraphicsContext3DQt
Comment 20 Simon Hausmann 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?
Comment 21 Noam Rosenthal 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.
Comment 22 Noam Rosenthal 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.
Comment 23 Simon Hausmann 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 :)
Comment 24 Helder Correia 2012-07-26 01:58:25 PDT
Created attachment 154580 [details]
GraphicsContext3DQt
Comment 25 Helder Correia 2012-07-26 02:02:03 PDT
Created attachment 154582 [details]
TextureMapperGL
Comment 26 Early Warning System Bot 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
Comment 27 Early Warning System Bot 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
Comment 28 Simon Hausmann 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?
Comment 29 Gustavo Noronha (kov) 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
Comment 30 Helder Correia 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.
Comment 31 Helder Correia 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?
Comment 32 Noam Rosenthal 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.
Comment 33 Helder Correia 2012-07-26 12:55:56 PDT
Created attachment 154725 [details]
GraphicsContext3DQt
Comment 34 Helder Correia 2012-07-26 12:57:50 PDT
Created attachment 154727 [details]
TextureMapperGL
Comment 35 Helder Correia 2012-07-26 16:02:00 PDT
Created attachment 154777 [details]
RenderToCurrentGLContext/GC3DQt
Comment 36 Helder Correia 2012-07-26 16:13:17 PDT
Created attachment 154782 [details]
texImage2DDirect
Comment 37 Helder Correia 2012-07-26 16:15:14 PDT
Created attachment 154783 [details]
TextureMapperGL
Comment 38 Noam Rosenthal 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.
Comment 39 Helder Correia 2012-07-26 16:46:39 PDT
Created attachment 154787 [details]
TextureMapperGL
Comment 40 Noam Rosenthal 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.
Comment 41 WebKit Review Bot 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
Comment 42 Helder Correia 2012-07-26 19:55:42 PDT
Created attachment 154818 [details]
RenderToCurrentGLContext/GC3DQt - try fix Cr
Comment 43 Helder Correia 2012-07-26 20:02:49 PDT
Created attachment 154819 [details]
Unified patch for EWS
Comment 44 Early Warning System Bot 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
Comment 45 Early Warning System Bot 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
Comment 46 WebKit Review Bot 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.
Comment 47 Roland Takacs 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 :)
Comment 48 Kenneth Rohde Christiansen 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() ?
Comment 49 Noam Rosenthal 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 :)
Comment 50 Roland Takacs 2012-08-15 12:56:30 PDT
Created attachment 158621 [details]
patch
Comment 51 Kenneth Rohde Christiansen 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?
Comment 52 Roland Takacs 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)
Comment 53 Roland Takacs 2012-08-22 03:55:12 PDT
Created attachment 159896 [details]
patch

Tested on GTK. It works!
Comment 54 Noam Rosenthal 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?
Comment 55 Roland Takacs 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 :)
Comment 56 Simon Hausmann 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/
Comment 57 Roland Takacs 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]
Comment 58 Noam Rosenthal 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 ;
Comment 59 Roland Takacs 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.
Comment 60 Roland Takacs 2012-08-24 15:08:39 PDT
Created attachment 160508 [details]
patch [TexImage2DDirect]
Comment 61 Roland Takacs 2012-08-24 15:22:58 PDT
Created attachment 160511 [details]
patch [TexImage2DDirect]
Comment 62 WebKit Review Bot 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>
Comment 63 WebKit Review Bot 2012-08-24 15:30:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 64 Noam Rosenthal 2012-08-24 18:04:03 PDT
Still missing some patches.
Comment 65 WebKit Review Bot 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
Comment 66 Noam Rosenthal 2012-08-24 18:14:19 PDT
Created attachment 160535 [details]
patch [TexImage2DDirect]
Comment 67 WebKit Review Bot 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>
Comment 68 WebKit Review Bot 2012-08-24 19:25:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 69 Roland Takacs 2012-08-25 03:24:54 PDT
Created attachment 160559 [details]
patch [TextureMapperGL]

final part of the big patch.
Comment 70 Noam Rosenthal 2012-08-25 09:27:17 PDT
Last patch to go
Comment 71 Noam Rosenthal 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.
Comment 72 Roland Takacs 2012-08-25 12:10:48 PDT
Created attachment 160567 [details]
patch [TextureMapperGL]
Comment 73 WebKit Review Bot 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>
Comment 74 WebKit Review Bot 2012-08-25 18:51:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 75 Csaba Osztrogonác 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
Comment 76 WebKit Review Bot 2012-08-27 03:45:20 PDT
Re-opened since this is blocked by 95069
Comment 77 Roland Takacs 2012-08-28 08:26:17 PDT
Created attachment 160989 [details]
patch

fixed patch
Comment 78 Csaba Osztrogonác 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?
Comment 79 Csaba Osztrogonác 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);
Comment 80 Roland Takacs 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 :)
Comment 81 Martin Robinson 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.
Comment 82 Martin Robinson 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.
Comment 83 Noam Rosenthal 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.
Comment 84 WebKit Review Bot 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>
Comment 85 WebKit Review Bot 2012-08-29 16:12:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 86 Csaba Osztrogonác 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.)