Bug 78672 – [Texmap] Move TextureMapperGL to use GraphicsContext3D

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

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.

[Yael]

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

[Noam Rosenthal]

(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]

(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]

> 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]

Created attachment 153650 [details]
Patch

[Helder Correia]

@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 on attachment 153650 [details]
Patch

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

[Martin Robinson]

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]

Created attachment 153653 [details]
Try fix Qt4 build

[Early Warning System Bot]

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]

Created attachment 153657 [details]
Try fix Qt4 build

[Early Warning System Bot]

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]

(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 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]

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

[Noam Rosenthal]

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]

Created attachment 153896 [details]
Part 1 - GraphicsContext3DQt

[Helder Correia]

Created attachment 153906 [details]
TextureMapperGL

[Helder Correia]

Created attachment 153909 [details]
GraphicsContext3DQt

[Simon Hausmann]

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]

(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]

(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]

(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]

Created attachment 154580 [details]
GraphicsContext3DQt

[Helder Correia]

Created attachment 154582 [details]
TextureMapperGL

[Early Warning System Bot]

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 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 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 on attachment 154582 [details]
TextureMapperGL

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

[Helder Correia]

(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]

> > > 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]

(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]

Created attachment 154725 [details]
GraphicsContext3DQt

[Helder Correia]

Created attachment 154727 [details]
TextureMapperGL

[Helder Correia]

Created attachment 154777 [details]
RenderToCurrentGLContext/GC3DQt

[Helder Correia]

Created attachment 154782 [details]
texImage2DDirect

[Helder Correia]

Created attachment 154783 [details]
TextureMapperGL

[Noam Rosenthal]

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]

Created attachment 154787 [details]
TextureMapperGL

[Noam Rosenthal]

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 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]

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

[Helder Correia]

Created attachment 154819 [details]
Unified patch for EWS

[Early Warning System Bot]

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 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 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]

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 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
> +
> [phone]  Helder Correia  <[e-mail]>
> +
> +        [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 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]

Created attachment 158621 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 158621 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158621&action=review

> Source/WebCore/ChangeLog:1
> [phone]  Helder Correia  <[e-mail]>

Shouldnt this be your name?

[Roland Takacs]

(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
> > [phone]  Helder Correia  <[e-mail]>
> 
> 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 ([e-mail])

[Roland Takacs]

Created attachment 159896 [details]
patch

Tested on GTK. It works!

[Noam Rosenthal]

(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]

(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]

(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]

(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 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]

Created attachment 160499 [details]
patch [RenderToCurrentGLContext]

part 1 - I did not find issues with this patch so it works.

[Roland Takacs]

Created attachment 160508 [details]
patch [TexImage2DDirect]

[Roland Takacs]

Created attachment 160511 [details]
patch [TexImage2DDirect]

[WebKit Review Bot]

Comment on attachment 160499 [details]
patch [RenderToCurrentGLContext]

Clearing flags on attachment: 160499

Committed r126635: <http://trac.webkit.org/changeset/126635>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

Still missing some patches.

[WebKit Review Bot]

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]

Created attachment 160535 [details]
patch [TexImage2DDirect]

[WebKit Review Bot]

Comment on attachment 160535 [details]
patch [TexImage2DDirect]

Clearing flags on attachment: 160535

Committed r126675: <http://trac.webkit.org/changeset/126675>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roland Takacs]

Created attachment 160559 [details]
patch [TextureMapperGL]

final part of the big patch.

[Noam Rosenthal]

Last patch to go

[Noam Rosenthal]

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]

Created attachment 160567 [details]
patch [TextureMapperGL]

[WebKit Review Bot]

Comment on attachment 160567 [details]
patch [TextureMapperGL]

Clearing flags on attachment: 160567

Committed r126694: <http://trac.webkit.org/changeset/126694>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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]

Re-opened since this is blocked by 95069

[Roland Takacs]

Created attachment 160989 [details]
patch

fixed patch

[Csaba Osztrogonác]

(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]

(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]

(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 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 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 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 on attachment 160989 [details]
patch

Clearing flags on attachment: 160989

Committed r127063: <http://trac.webkit.org/changeset/127063>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.)