Bug 53201 – Make GraphicsContext3D use DrawingBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Chris Marrin]

Right now GC3D creates its own layer and manages its own multisample buffers. But this functionality is also done in DrawingBuffer (currently used by Chrome's accelerated 2D rendering). Getting rid of this code in GC3D and using a DrawingBuffer instead not only cleans up the code, but will make it easier to port WebGL to Windows.

[Chris Marrin]

Created attachment 80275 [details]
Patch

[Chris Marrin]

Comment on attachment 80275 [details]
Patch

This code causes 8 WebGL LayoutTests to fail. 6 tests are due to the antialiasing logic, because when AA is turned off they pass. The other 2 seem to have something to do with buffer initialization because they happen in both cases.

The tests that fail in both cases are:

fast/canvas/webgl/context-lost-restored.html
fast/canvas/webgl/uninitialized-test.html

The tests that fail on with AA on are:

fast/canvas/webgl/canvas-test.html
fast/canvas/webgl/copy-tex-image-and-sub-image-2d.html
fast/canvas/webgl/gl-pixelstorei.html
fast/canvas/webgl/invalid-passed-params.html
fast/canvas/webgl/renderbuffer-initialization.html
fast/canvas/webgl/tex-input-validation.html

I will get back to this when there is more time

[Jeff Timanus]

Did you ever have time to get back to this issue?  

Were there other issues (than the reported test failures) that blocked this change?

I started to work on a similar patch, but was directed here.  Would it be reasonable for me to pick up your change, and carry it through?

[Jeff Timanus]

Created attachment 101643 [details]
Patch

[Jeff Timanus]

Patch 101643 continues this work, but only supporting Chromium.

A DrawingBuffer instance is created (only on Chromium platforms) during construction of WebGLRenderingContexts.  WebGLRenderingContext methods   conditionally forward rendering to the DrawingBuffer.

This is more of a prototype/proof of concept patch, as there are still some fairly large inconsistencies in the design:
  - DrawingBuffer has routines for constructing a platformLayer.  In Chromium, these always construct Canvas2DLayerChromium instances, which are not compatible with WebGL.  The WebGL path constructs its platform layer via GraphicsContext3D::platformLayer.  These competing access points for platform-layer construction need to be unified.
  - Because of the dependency on the DrawingBuffer owning a platformLayer (which it does not when constructed for WebGL use), I added a texture-id parameter to publishToPlatformLayer(...) so that WebGLLayerChromium::updateCompositorResources could provide an appropriate value.

I don't know the history of the design of this class, but I believe that DrawingBuffer should not be responsible for construction/management of platform layer objects.

Please review and comment on these thoughts.

[Jeff Timanus]

Comment on attachment 101643 [details]
Patch

This patch produces the following WebGL layout test (chromium-windows) failures:

Regressions: Unexpected text diff mismatch : (4)
  fast/canvas/webgl/canvas-test.html = TEXT
  fast/canvas/webgl/drawingbuffer-test.html = TEXT
  fast/canvas/webgl/gl-pixelstorei.html = TEXT
  fast/canvas/webgl/uninitialized-test.html = TEXT

Regressions: Unexpected image mismatch : (2)
  fast/canvas/webgl/css-webkit-canvas-repaint.html = IMAGE
  fast/canvas/webgl/css-webkit-canvas.html = IMAGE

Regressions: Unexpected tests timed out : (1)
  fast/canvas/webgl/premultiplyalpha-test.html = TIMEOUT

[WebKit Review Bot]

Comment on attachment 101643 [details]
Patch

Attachment 101643 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9208883

New failing tests:
fast/canvas/webgl/premultiplyalpha-test.html
fast/canvas/webgl/uninitialized-test.html
fast/canvas/webgl/drawingbuffer-test.html
fast/canvas/webgl/canvas-test.html
fast/canvas/webgl/css-webkit-canvas.html
fast/canvas/webgl/css-webkit-canvas-repaint.html
fast/canvas/webgl/gl-pixelstorei.html

[Kenneth Russell]

(In reply to comment #5)
> Patch 101643 continues this work, but only supporting Chromium.
> 
> A DrawingBuffer instance is created (only on Chromium platforms) during construction of WebGLRenderingContexts.  WebGLRenderingContext methods   conditionally forward rendering to the DrawingBuffer.
> 
> This is more of a prototype/proof of concept patch, as there are still some fairly large inconsistencies in the design:
>   - DrawingBuffer has routines for constructing a platformLayer.  In Chromium, these always construct Canvas2DLayerChromium instances, which are not compatible with WebGL.  The WebGL path constructs its platform layer via GraphicsContext3D::platformLayer.  These competing access points for platform-layer construction need to be unified.
>   - Because of the dependency on the DrawingBuffer owning a platformLayer (which it does not when constructed for WebGL use), I added a texture-id parameter to publishToPlatformLayer(...) so that WebGLLayerChromium::updateCompositorResources could provide an appropriate value.
> 
> I don't know the history of the design of this class, but I believe that DrawingBuffer should not be responsible for construction/management of platform layer objects.

The fact that both GraphicsContext3D and DrawingBuffer create PlatformLayers is definitely an inconsistency that needs to be unified. The direction I would personally like to see things move is to remove the PlatformLayer from GraphicsContext3D. Conceptually, the buffer that the GraphicsContext3D draws into should be a separate concern from the context (as in true OpenGL). DrawingBuffer seems as good a place as any to host the layer, because any entity (accelerated 2D canvas, WebGL) using a DrawingBuffer will want that DrawingBuffer hosted in its own layer.

Which way would you rather see the platform layer managed?

We could add some information to the DrawingBuffer creation methods to indicate for what purpose it's being allocated, in the case that we need a different platform layer type depending on whether the DrawingBuffer is being used for the 2D canvas or WebGL.

[Jeff Timanus]

(In reply to comment #8)
> (In reply to comment #5)
> > Patch 101643 continues this work, but only supporting Chromium.
> > 
> > A DrawingBuffer instance is created (only on Chromium platforms) during construction of WebGLRenderingContexts.  WebGLRenderingContext methods   conditionally forward rendering to the DrawingBuffer.
> > 
> > This is more of a prototype/proof of concept patch, as there are still some fairly large inconsistencies in the design:
> >   - DrawingBuffer has routines for constructing a platformLayer.  In Chromium, these always construct Canvas2DLayerChromium instances, which are not compatible with WebGL.  The WebGL path constructs its platform layer via GraphicsContext3D::platformLayer.  These competing access points for platform-layer construction need to be unified.
> >   - Because of the dependency on the DrawingBuffer owning a platformLayer (which it does not when constructed for WebGL use), I added a texture-id parameter to publishToPlatformLayer(...) so that WebGLLayerChromium::updateCompositorResources could provide an appropriate value.
> > 
> > I don't know the history of the design of this class, but I believe that DrawingBuffer should not be responsible for construction/management of platform layer objects.
> 
> The fact that both GraphicsContext3D and DrawingBuffer create PlatformLayers is definitely an inconsistency that needs to be unified. The direction I would personally like to see things move is to remove the PlatformLayer from GraphicsContext3D. Conceptually, the buffer that the GraphicsContext3D draws into should be a separate concern from the context (as in true OpenGL). DrawingBuffer seems as good a place as any to host the layer, because any entity (accelerated 2D canvas, WebGL) using a DrawingBuffer will want that DrawingBuffer hosted in its own layer.
> 
> Which way would you rather see the platform layer managed?
> 
> We could add some information to the DrawingBuffer creation methods to indicate for what purpose it's being allocated, in the case that we need a different platform layer type depending on whether the DrawingBuffer is being used for the 2D canvas or WebGL.

I agree with your above suggestion.  My thoughts are the following:
  - Remove platformLayer() routine from GraphicsContext3D, and delegate all platformLayer construction, and lifetime management duties to the DrawingBuffer.
  - DrawingBuffer can introduce a new virtual interface, accepted during construction, that can be used as a delegate for constructing the appropriate type of PlatformLayer.  (This should be ok, because a canvas cannot switch from 2D to 3D, so we do not have to worry about a DrawingBuffer being similarly converted.)

My concern at the moment is that this change will be invasive to the other WebKit ports.  Is there an approved work-flow when making changes that require touching multiple ports?  Should the first pass only apply to Chromium builds of WebKit, at the potential expense of clarity of software design?

[Kenneth Russell]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #5)
> > > Patch 101643 continues this work, but only supporting Chromium.
> > > 
> > > A DrawingBuffer instance is created (only on Chromium platforms) during construction of WebGLRenderingContexts.  WebGLRenderingContext methods   conditionally forward rendering to the DrawingBuffer.
> > > 
> > > This is more of a prototype/proof of concept patch, as there are still some fairly large inconsistencies in the design:
> > >   - DrawingBuffer has routines for constructing a platformLayer.  In Chromium, these always construct Canvas2DLayerChromium instances, which are not compatible with WebGL.  The WebGL path constructs its platform layer via GraphicsContext3D::platformLayer.  These competing access points for platform-layer construction need to be unified.
> > >   - Because of the dependency on the DrawingBuffer owning a platformLayer (which it does not when constructed for WebGL use), I added a texture-id parameter to publishToPlatformLayer(...) so that WebGLLayerChromium::updateCompositorResources could provide an appropriate value.
> > > 
> > > I don't know the history of the design of this class, but I believe that DrawingBuffer should not be responsible for construction/management of platform layer objects.
> > 
> > The fact that both GraphicsContext3D and DrawingBuffer create PlatformLayers is definitely an inconsistency that needs to be unified. The direction I would personally like to see things move is to remove the PlatformLayer from GraphicsContext3D. Conceptually, the buffer that the GraphicsContext3D draws into should be a separate concern from the context (as in true OpenGL). DrawingBuffer seems as good a place as any to host the layer, because any entity (accelerated 2D canvas, WebGL) using a DrawingBuffer will want that DrawingBuffer hosted in its own layer.
> > 
> > Which way would you rather see the platform layer managed?
> > 
> > We could add some information to the DrawingBuffer creation methods to indicate for what purpose it's being allocated, in the case that we need a different platform layer type depending on whether the DrawingBuffer is being used for the 2D canvas or WebGL.
> 
> I agree with your above suggestion.  My thoughts are the following:
>   - Remove platformLayer() routine from GraphicsContext3D, and delegate all platformLayer construction, and lifetime management duties to the DrawingBuffer.
>   - DrawingBuffer can introduce a new virtual interface, accepted during construction, that can be used as a delegate for constructing the appropriate type of PlatformLayer.  (This should be ok, because a canvas cannot switch from 2D to 3D, so we do not have to worry about a DrawingBuffer being similarly converted.)
> 
> My concern at the moment is that this change will be invasive to the other WebKit ports.  Is there an approved work-flow when making changes that require touching multiple ports?  Should the first pass only apply to Chromium builds of WebKit, at the potential expense of clarity of software design?

That sounds like a good plan. You could #ifndef PLATFORM(CHROMIUM) the removal of platformLayer() from GraphicsContext3D, for example. Once you have everything working you could advertise the new facility to the other ports. DrawingBuffer doesn't currently have much if any use outside of the Chromium port at the moment, so you don't need to worry as much about breakage there.

[John Bauman]

Jeff, could you also make sure that fast/canvas/buffer-preserve-test.html still works before you check anything in (or at least before you call this project finished)? It doesn't work properly in DRT (issues with the webgraphicscontext implementation), so you'll need to run it manually in chromium.

[Jeff Timanus]

(In reply to comment #11)
> Jeff, could you also make sure that fast/canvas/buffer-preserve-test.html still works before you check anything in (or at least before you call this project finished)? It doesn't work properly in DRT (issues with the webgraphicscontext implementation), so you'll need to run it manually in chromium.

I've managed to fix all of the layout test failures mentioned above.  I'm looking at the buffer-preserve-test.html, but there is no layout test with that name under fast/canvas.  

Did you mean the following test?
/third_party/webgl_conformance/conformance/buffer-preserve-test.html

Note that it is failing with my change, so I'm investigating.

[Jeff Timanus]

Created attachment 104090 [details]
Patch

[James Robinson]

Comment on attachment 104090 [details]
Patch

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

There's lots of code in here behind #if 0.  Is this not quite finished? R- since this isn't ready to land, but perhaps you were looking for initial feedback?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:599
> +#if 0  // TODO:  Why is this code here?
>      if (!m_markedCanvasDirty && !m_layerCleared)
>          return;
> +#endif

you don't mean to commit this, do you?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:77
> +
> +#if 0
> +    updatePlatformTexture();
> +#endif
> +

is this meant just for testing or something?

> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp:89
> +#if 0
> +void CanvasLayerChromium::setTextureChanged()
> +{
> +    m_textureChanged = true;
> +}
> +#endif

why the #if 0?

> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp:110
> +#if 0

huh?

[Jeff Timanus]

(In reply to comment #13)
> Created an attachment (id=104090) [details]
> Patch

Note:  This patch isn't quite complete.  I had all of the layout tests passing, along with the WebGL conformance tests, but the removal of copyTextureToParentTextureCHROMIUM introduced non trivial regressions.

Please do not remark on style issues, as I will resolve them in the next upload.  This patch is just to demonstrate the high level approach.

There are a few points that I'd like to have some feedback on at this point, so I know that I've taken things in the right direction.
  - Does the new compositor model of directly rendering from the PlatformLayer's texture work for WebGL?  Are there constraints that will not allow this model?  I briefly tried removing the explicit m_textureId from WebGLPlatformLayer, but observed regressions.
  - Because I want to remove the call to GraphicsContext3D::prepareTexture, and replace it with DrawingBuffer::publishToPlatformLayer, the WebGL path will now need to follow the same compositing path as Canvas2D.  Since copyTextureToParentTextureCHROMIUM has been removed, is it sufficient to replace the previous copy call with a glCopyTexImage2D from the DrawingBuffer's fbo to the PlatformLayer's texture?  This is the approach that I'm considering moving forward with.
  - All of the readBackFramebuffer(...) calls were always reading from fbo 0, which is totally incorrect when the target of the rendering is to a fbo managed by the drawing buffer.  As a stop-gap, I added a bindDefaultBackbuffer argument to this set of routines to specify if the read should be from the true back-buffer, or the currently bound fbo.  This change requires a parallel change in chrome.  See first-pass CL:  http://codereview.chromium.org/7669002

Input on these concerns, or other architectural issues that I haven't yet observed is much appreciated!

[Jeff Timanus]

(In reply to comment #14)
> (From update of attachment 104090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104090&action=review
> 
> There's lots of code in here behind #if 0.  Is this not quite finished? R- since this isn't ready to land, but perhaps you were looking for initial feedback?

Yes, this patch is just for high-level review and feedback.  I left some #if 0's in there because I'm not certain if this is the correct final approach.

Will use the --no-review flag the next time I upload a patch like this.

> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:599
> > +#if 0  // TODO:  Why is this code here?
> >      if (!m_markedCanvasDirty && !m_layerCleared)
> >          return;
> > +#endif
> 
> you don't mean to commit this, do you?
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:77
> > +
> > +#if 0
> > +    updatePlatformTexture();
> > +#endif
> > +
> 
> is this meant just for testing or something?
> 
> > Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp:89
> > +#if 0
> > +void CanvasLayerChromium::setTextureChanged()
> > +{
> > +    m_textureChanged = true;
> > +}
> > +#endif
> 
> why the #if 0?
> 
> > Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp:110
> > +#if 0
> 
> huh?

[Kenneth Russell]

Comment on attachment 104090 [details]
Patch

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

The general direction looks good. A couple of high-level comments. First, we need to make sure that WebGL applications can't query the framebuffer in the DrawingBuffer or any of its framebuffer or renderbuffer attachments. This may involve some more logic in WebGLRenderingContext::getParameter -- not 100% sure. Also, we need to make sure that we are not unnecessarily resizing the command buffer service's default frame buffer to anything except a size of 1x1. I am pretty sure that more changes will be needed beyond what you've got here to avoid this.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1144
> +            m_drawingBuffer->commit();

jbauman recently fixed bugs in Chromium's command buffer service-side code where copyTexImage2D, readPixels and a couple of other entry points were incorrectly committing the frame buffer to the compositor, causing the (cleared) WebGL back buffer to be displayed to the screen after printing. We need to make sure not to regress this functionality. The previously needed fix was to resolve the multisampled frame buffer to a temporary buffer rather than the buffer shared with the compositor. Another part of the fix was to be able to capture the compositor's copy of the results for printing.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:479
> +    // The Chromium port manages PlatformLayer instaces via the DrawingBuffer
> +    // class.
> +    PlatformLayer* platformLayer() const { return 0; }

Then shouldn't this change be bracketed with #if PLATFORM(CHROMIUM)?

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:167
> +            m_platformLayer = WebGLLayerChromium::create(0);

ASSERT that m_usage == DrawingBufferUsageWebGL?

[Stephen White]

Comment on attachment 104090 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:145
> +        m_context->activeTexture(GraphicsContext3D::TEXTURE0);
> +        m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, parentTexture);

Do we need to worry about restoring the formerly-bound and active textures for WebGL here?  Or do they save their own state somewhere else?

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:146
> +        m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,

I know it's not new to this patch, but it'd be really nice to try to eliminate this copy for WebGL, just as we did for 2D canvas.  Not sure how to go about that.

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:155
> +    if (multisampleBound)

Nit:  could probably call multisample() again here instead of stashing it in a local (it's cheap).

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:101
> +        context->finish();

I think flush() may now have fence semantics, and may be usable here.  Check with jbates.

[John Bauman]

Comment on attachment 104090 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:146
>> +        m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,
> 
> I know it's not new to this patch, but it'd be really nice to try to eliminate this copy for WebGL, just as we did for 2D canvas.  Not sure how to go about that.

In theory we don't need to do a copy when preserveDrawingBuffer=true, and when it's false we could try flipping the drawing buffer texture with the compositor texture.

However, I'm not sure how this copyTexImage2D is supposed to work. I thought that WebGL contexts are in a different share group from the compositor, so they're in different namespaces and can't bind the parent texture to change it. Has that changed?

[Kenneth Russell]

Comment on attachment 104090 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:146
>>> +        m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,
>> 
>> I know it's not new to this patch, but it'd be really nice to try to eliminate this copy for WebGL, just as we did for 2D canvas.  Not sure how to go about that.
> 
> In theory we don't need to do a copy when preserveDrawingBuffer=true, and when it's false we could try flipping the drawing buffer texture with the compositor texture.
> 
> However, I'm not sure how this copyTexImage2D is supposed to work. I thought that WebGL contexts are in a different share group from the compositor, so they're in different namespaces and can't bind the parent texture to change it. Has that changed?

This is a good point. Nothing has changed in this regard, so there is likely a problem here.

[James Robinson]

(In reply to comment #20)
> (From update of attachment 104090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104090&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:146
> >>> +        m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,
> >> 
> >> I know it's not new to this patch, but it'd be really nice to try to eliminate this copy for WebGL, just as we did for 2D canvas.  Not sure how to go about that.
> > 
> > In theory we don't need to do a copy when preserveDrawingBuffer=true, and when it's false we could try flipping the drawing buffer texture with the compositor texture.
> > 
> > However, I'm not sure how this copyTexImage2D is supposed to work. I thought that WebGL contexts are in a different share group from the compositor, so they're in different namespaces and can't bind the parent texture to change it. Has that changed?
> 
> This is a good point. Nothing has changed in this regard, so there is likely a problem here.

That matches my understanding.  In chromium, WebGL contexts are not in the same share group as the compositor and so you cannot CopyTexImage2D between them.

[Stephen White]

(In reply to comment #20)
> (From update of attachment 104090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104090&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:146
> >>> +        m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,
> >> 
> >> I know it's not new to this patch, but it'd be really nice to try to eliminate this copy for WebGL, just as we did for 2D canvas.  Not sure how to go about that.
> > 
> > In theory we don't need to do a copy when preserveDrawingBuffer=true, and when it's false we could try flipping the drawing buffer texture with the compositor texture.
> > 
> > However, I'm not sure how this copyTexImage2D is supposed to work. I thought that WebGL contexts are in a different share group from the compositor, so they're in different namespaces and can't bind the parent texture to change it. Has that changed?
> 
> This is a good point. Nothing has changed in this regard, so there is likely a problem here.

I had thought that there was one "special" texture that was shared by both WebGL and the compositor in the service side of the command buffers.  Isn't that how the copy in the SwapBuffers implementation works in the current implementation?  Or is there some other magic there?

[James Robinson]

Right, that's how GraphicsContext3D::prepareTexture().  That is done using 100% service-side magic, not share groups (which are exposed client side).

[Jeff Timanus]

Comment on attachment 104090 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1144
>> +            m_drawingBuffer->commit();
> 
> jbauman recently fixed bugs in Chromium's command buffer service-side code where copyTexImage2D, readPixels and a couple of other entry points were incorrectly committing the frame buffer to the compositor, causing the (cleared) WebGL back buffer to be displayed to the screen after printing. We need to make sure not to regress this functionality. The previously needed fix was to resolve the multisampled frame buffer to a temporary buffer rather than the buffer shared with the compositor. Another part of the fix was to be able to capture the compositor's copy of the results for printing.

The drawing-buffer is doing a commit to a separate FBO that is managed entirely by the the drawing-buffer.  This commit should not be pushing contents to the compositor.

The introduction of these drawing buffer commits should not change the logic present in the service side handling of copyTexImage2D, readPixels or others.

Is there a bug I can look for to double-check the printing repro-steps, just to double check?

>> Source/WebCore/platform/graphics/GraphicsContext3D.h:479
>> +    PlatformLayer* platformLayer() const { return 0; }
> 
> Then shouldn't this change be bracketed with #if PLATFORM(CHROMIUM)?

Yes.  It should be bracketed with the #if. 

Done.

>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:145
>> +        m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, parentTexture);
> 
> Do we need to worry about restoring the formerly-bound and active textures for WebGL here?  Or do they save their own state somewhere else?

Good point.

I added this code in an attempt to replace the previous calls to copyTextureToParentTextureCHROMIUM.  

You are right that this code is incorrect in that it does not properly save/restore the active texture.  I do not believe that the state is saved elsewhere.

>>>>>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:146
>>>>>> +        m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,
>>>>> 
>>>>> I know it's not new to this patch, but it'd be really nice to try to eliminate this copy for WebGL, just as we did for 2D canvas.  Not sure how to go about that.
>>>> 
>>>> In theory we don't need to do a copy when preserveDrawingBuffer=true, and when it's false we could try flipping the drawing buffer texture with the compositor texture.
>>>> 
>>>> However, I'm not sure how this copyTexImage2D is supposed to work. I thought that WebGL contexts are in a different share group from the compositor, so they're in different namespaces and can't bind the parent texture to change it. Has that changed?
>>> 
>>> This is a good point. Nothing has changed in this regard, so there is likely a problem here.
>> 
>> That matches my understanding.  In chromium, WebGL contexts are not in the same share group as the compositor and so you cannot CopyTexImage2D between them.
> 
> I had thought that there was one "special" texture that was shared by both WebGL and the compositor in the service side of the command buffers.  Isn't that how the copy in the SwapBuffers implementation works in the current implementation?  Or is there some other magic there?

If the copyTexImage2D approach is not suitable, would it be reasonable to re-introduce copyTextureToParentTextureCHROMIUM?

If not, what do we need to do to allow either this copyTexImage2D to succeed, or to bypass the copy entirely, and have the compositor read from the DrawingBuffer's FBO, as presently happens for Canvas2D content?

Just to note, before copyTextureToParentTextureCHROMIUM was deprecated and removed, I had all of the tests (layout + conformance) passing.

>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:155
>> +    if (multisampleBound)
> 
> Nit:  could probably call multisample() again here instead of stashing it in a local (it's cheap).

An easy change.  Done.

>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:167
>> +            m_platformLayer = WebGLLayerChromium::create(0);
> 
> ASSERT that m_usage == DrawingBufferUsageWebGL?

Done.

>> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:101
>> +        context->finish();
> 
> I think flush() may now have fence semantics, and may be usable here.  Check with jbates.

This code is in this form because I had initially tried to unify the texture management between Canvas2DLayerChromium, and WebGLLayerChromium.  Both were performing similar operations to prepare the parent texture, except that the Canvas2DLayerChromium implementation performed the extra active texture and flush calls. 

Because the Canvas2DLayer no longer performs any of this setup, and to minimize the changes introduced by this patch, I'll revert the texture preparation code here to its previous state.

[James Robinson]

(In reply to comment #24)
> >>>>>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:146
> >>>>>> +        m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,
> >>>>> 
> >>>>> I know it's not new to this patch, but it'd be really nice to try to eliminate this copy for WebGL, just as we did for 2D canvas.  Not sure how to go about that.
> >>>> 
> >>>> In theory we don't need to do a copy when preserveDrawingBuffer=true, and when it's false we could try flipping the drawing buffer texture with the compositor texture.
> >>>> 
> >>>> However, I'm not sure how this copyTexImage2D is supposed to work. I thought that WebGL contexts are in a different share group from the compositor, so they're in different namespaces and can't bind the parent texture to change it. Has that changed?
> >>> 
> >>> This is a good point. Nothing has changed in this regard, so there is likely a problem here.
> >> 
> >> That matches my understanding.  In chromium, WebGL contexts are not in the same share group as the compositor and so you cannot CopyTexImage2D between them.
> > 
> > I had thought that there was one "special" texture that was shared by both WebGL and the compositor in the service side of the command buffers.  Isn't that how the copy in the SwapBuffers implementation works in the current implementation?  Or is there some other magic there?
> 
> If the copyTexImage2D approach is not suitable, would it be reasonable to re-introduce copyTextureToParentTextureCHROMIUM?
> 
> If not, what do we need to do to allow either this copyTexImage2D to succeed, or to bypass the copy entirely, and have the compositor read from the DrawingBuffer's FBO, as presently happens for Canvas2D content?
> 
> Just to note, before copyTextureToParentTextureCHROMIUM was deprecated and removed, I had all of the tests (layout + conformance) passing.
> 

copyTextureToParentTextureCHROMIUM is not ideal since it implies at least one unnecessary texture copy in the multisampled case, and potentially an extra copy in all cases.

To back up a bit, the issue here is all about resource sharing between the WebGL context and the compositor context.  Different implementations of GraphicsContext3D may handle this in different ways, but in chromium today we do not support arbitrary resource sharing between these two.  prepareTexture() is wired up in chromium to do a texture copy (or multisample resolve) to a texture that is exposed to the compositor context.  In that sense, it's a form of service-side "magic" that allows a very limited form of resource sharing.  copyTextureToParentTextureCHROMIUM() was another form of service-side magic to support sharing (via a copy) a given texture with a texture in the compositor texture.  The approach was fundamentally broken, however, due to fact that it relied on a parent->child relationship between canvas contexts and compositor contexts, but that doesn't exist in reality.  A given WebGL <canvas> can migrate between different tabs, or between a window and a popup, which use different compositor contexts.

We could expose another entry point to allow sharing of a specific resource and a given compositor context resource.  For WebGL we only need this for framebuffer 0 so something like prepareTexture() makes sense.  I'm not completely sure what the best form of this API is, but whatever it is it shouldn't force us to do extra copies and it definitely cannot rely on a fixed parent-child relationship.

HTH

[Chris Marrin]

(In reply to comment #25)
>...
> To back up a bit, the issue here is all about resource sharing between the WebGL context and the compositor context.  Different implementations of GraphicsContext3D may handle this in different ways, but in chromium today we do not support arbitrary resource sharing between these two.  prepareTexture() is wired up in chromium to do a texture copy (or multisample resolve) to a texture that is exposed to the compositor context.  In that sense, it's a form of service-side "magic" that allows a very limited form of resource sharing.  copyTextureToParentTextureCHROMIUM() was another form of service-side magic to support sharing (via a copy) a given texture with a texture in the compositor texture.  The approach was fundamentally broken, however, due to fact that it relied on a parent->child relationship between canvas contexts and compositor contexts, but that doesn't exist in reality.  A given WebGL <canvas> can migrate between different tabs, or between a window and a popup, which use different compositor contexts.
> 
> We could expose another entry point to allow sharing of a specific resource and a given compositor context resource.  For WebGL we only need this for framebuffer 0 so something like prepareTexture() makes sense.  I'm not completely sure what the best form of this API is, but whatever it is it shouldn't force us to do extra copies and it definitely cannot rely on a fixed parent-child relationship.

I've never liked the name "prepareTexture". Prepare it for what? From its use (at least in the Mac implementation), it ids used to get the render buffer into a state where it can be composited. This includes resolving in the AA case. In the Mac implementation, this also includes copying the frame buffer into the shared texture, but that's all Mac specific and internal. So it seems like we shouldn't even use the word "texture". It would probably be better named "prepareDrawingBufferForCompositing" and it should be a method on DrawingBuffer rather than GC3D.

[Chris Marrin]

> I've never liked the name "prepareTexture". Prepare it for what? From its use (at least in the Mac implementation), it ids used to get the render buffer into a state where it can be composited. This includes resolving in the AA case. In the Mac implementation, this also includes copying the frame buffer into the shared texture, but that's all Mac specific and internal. So it seems like we shouldn't even use the word "texture". It would probably be better named "prepareDrawingBufferForCompositing" and it should be a method on DrawingBuffer rather than GC3D.

And of course if it's moved to DrawingBuffer, it might be better named "prepareForCompositing".

[James Robinson]

(In reply to comment #27)
> > I've never liked the name "prepareTexture". Prepare it for what? From its use (at least in the Mac implementation), it ids used to get the render buffer into a state where it can be composited. This includes resolving in the AA case. In the Mac implementation, this also includes copying the frame buffer into the shared texture, but that's all Mac specific and internal. So it seems like we shouldn't even use the word "texture". It would probably be better named "prepareDrawingBufferForCompositing" and it should be a method on DrawingBuffer rather than GC3D.
> 
> And of course if it's moved to DrawingBuffer, it might be better named "prepareForCompositing".

That makes more sense.  I'm not sure where the name 'prepareTexture' came from, but as I understand it's a funny way to spell SwapBuffers.

[Jeff Timanus]

(In reply to comment #28)
> (In reply to comment #27)
> > > I've never liked the name "prepareTexture". Prepare it for what? From its use (at least in the Mac implementation), it ids used to get the render buffer into a state where it can be composited. This includes resolving in the AA case. In the Mac implementation, this also includes copying the frame buffer into the shared texture, but that's all Mac specific and internal. So it seems like we shouldn't even use the word "texture". It would probably be better named "prepareDrawingBufferForCompositing" and it should be a method on DrawingBuffer rather than GC3D.
> > 
> > And of course if it's moved to DrawingBuffer, it might be better named "prepareForCompositing".
> 
> That makes more sense.  I'm not sure where the name 'prepareTexture' came from, but as I understand it's a funny way to spell SwapBuffers.

(In reply to comment #28)
> (In reply to comment #27)
> > > I've never liked the name "prepareTexture". Prepare it for what? From its use (at least in the Mac implementation), it ids used to get the render buffer into a state where it can be composited. This includes resolving in the AA case. In the Mac implementation, this also includes copying the frame buffer into the shared texture, but that's all Mac specific and internal. So it seems like we shouldn't even use the word "texture". It would probably be better named "prepareDrawingBufferForCompositing" and it should be a method on DrawingBuffer rather than GC3D.
> > 
> > And of course if it's moved to DrawingBuffer, it might be better named "prepareForCompositing".
> 
> That makes more sense.  I'm not sure where the name 'prepareTexture' came from, but as I understand it's a funny way to spell SwapBuffers.

(In reply to comment #25)
> (In reply to comment #24)
> > >>>>>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:146
> > >>>>>> +        m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,
> > >>>>> 
> > >>>>> I know it's not new to this patch, but it'd be really nice to try to eliminate this copy for WebGL, just as we did for 2D canvas.  Not sure how to go about that.
> > >>>> 
> > >>>> In theory we don't need to do a copy when preserveDrawingBuffer=true, and when it's false we could try flipping the drawing buffer texture with the compositor texture.
> > >>>> 
> > >>>> However, I'm not sure how this copyTexImage2D is supposed to work. I thought that WebGL contexts are in a different share group from the compositor, so they're in different namespaces and can't bind the parent texture to change it. Has that changed?
> > >>> 
> > >>> This is a good point. Nothing has changed in this regard, so there is likely a problem here.
> > >> 
> > >> That matches my understanding.  In chromium, WebGL contexts are not in the same share group as the compositor and so you cannot CopyTexImage2D between them.
> > > 
> > > I had thought that there was one "special" texture that was shared by both WebGL and the compositor in the service side of the command buffers.  Isn't that how the copy in the SwapBuffers implementation works in the current implementation?  Or is there some other magic there?
> > 
> > If the copyTexImage2D approach is not suitable, would it be reasonable to re-introduce copyTextureToParentTextureCHROMIUM?
> > 
> > If not, what do we need to do to allow either this copyTexImage2D to succeed, or to bypass the copy entirely, and have the compositor read from the DrawingBuffer's FBO, as presently happens for Canvas2D content?
> > 
> > Just to note, before copyTextureToParentTextureCHROMIUM was deprecated and removed, I had all of the tests (layout + conformance) passing.
> > 
> 
> copyTextureToParentTextureCHROMIUM is not ideal since it implies at least one unnecessary texture copy in the multisampled case, and potentially an extra copy in all cases.
> 
> To back up a bit, the issue here is all about resource sharing between the WebGL context and the compositor context.  Different implementations of GraphicsContext3D may handle this in different ways, but in chromium today we do not support arbitrary resource sharing between these two.  prepareTexture() is wired up in chromium to do a texture copy (or multisample resolve) to a texture that is exposed to the compositor context.  In that sense, it's a form of service-side "magic" that allows a very limited form of resource sharing.  copyTextureToParentTextureCHROMIUM() was another form of service-side magic to support sharing (via a copy) a given texture with a texture in the compositor texture.  The approach was fundamentally broken, however, due to fact that it relied on a parent->child relationship between canvas contexts and compositor contexts, but that doesn't exist in reality.  A given WebGL <canvas> can migrate between different tabs, or between a window and a popup, which use different compositor contexts.
> 
> We could expose another entry point to allow sharing of a specific resource and a given compositor context resource.  For WebGL we only need this for framebuffer 0 so something like prepareTexture() makes sense.  I'm not completely sure what the best form of this API is, but whatever it is it shouldn't force us to do extra copies and it definitely cannot rely on a fixed parent-child relationship.

Thanks for the background, James.

I revisited the mechanics of the chromium swap-buffers implementation yesterday, hoping to see a clear way to push the contents from WebGL to the compositor, and I agree with your remarks on the service-side magic of swap-buffers, and the old copyTextureToParentTextureCHROMIUM.

Because I have been nursing this change for some time, I would like to get it in.  I would like to investigate and make a proposal for the sharing entry point that you suggest.  Has existing work already started on this?  Are there concerns relating to how the layer would be structured that haven't been touched upon already by this patch?  I don't want to tread on existing work that is already on-going.

On the other hand, I discussed this briefly with Vangelis, and he mentioned that we may want to hold off on making these changes until the threaded compositor is in place.  Can you comment on that suggestion?  Would the conversion to a threaded compositor have an impact on the design of a resource-sharing API/layer?

> 
> HTH

[James Robinson]

(In reply to comment #29)
> (In reply to comment #28)
> > To back up a bit, the issue here is all about resource sharing between the WebGL context and the compositor context.  Different implementations of GraphicsContext3D may handle this in different ways, but in chromium today we do not support arbitrary resource sharing between these two.  prepareTexture() is wired up in chromium to do a texture copy (or multisample resolve) to a texture that is exposed to the compositor context.  In that sense, it's a form of service-side "magic" that allows a very limited form of resource sharing.  copyTextureToParentTextureCHROMIUM() was another form of service-side magic to support sharing (via a copy) a given texture with a texture in the compositor texture.  The approach was fundamentally broken, however, due to fact that it relied on a parent->child relationship between canvas contexts and compositor contexts, but that doesn't exist in reality.  A given WebGL <canvas> can migrate between different tabs, or between a window and a popup, which use different compositor contexts.
> > 
> > We could expose another entry point to allow sharing of a specific resource and a given compositor context resource.  For WebGL we only need this for framebuffer 0 so something like prepareTexture() makes sense.  I'm not completely sure what the best form of this API is, but whatever it is it shouldn't force us to do extra copies and it definitely cannot rely on a fixed parent-child relationship.
> 
> Thanks for the background, James.
> 
> I revisited the mechanics of the chromium swap-buffers implementation yesterday, hoping to see a clear way to push the contents from WebGL to the compositor, and I agree with your remarks on the service-side magic of swap-buffers, and the old copyTextureToParentTextureCHROMIUM.
> 
> Because I have been nursing this change for some time, I would like to get it in.  I would like to investigate and make a proposal for the sharing entry point that you suggest.  Has existing work already started on this?  Are there concerns relating to how the layer would be structured that haven't been touched upon already by this patch?  I don't want to tread on existing work that is already on-going.

Well this patch just straight up does not work in chromium today, WebGL and the compositor are not in a share group so you can't copy a texture to another.  We need a new entry point.  So no, we can not just put this in.

> 
> On the other hand, I discussed this briefly with Vangelis, and he mentioned that we may want to hold off on making these changes until the threaded compositor is in place.  Can you comment on that suggestion?  Would the conversion to a threaded compositor have an impact on the design of a resource-sharing API/layer?

It has implications but I think they are solvable.  The place to start here is to figure out what API we need on GraphicsContext3D to enable resource sharing with the compositor.


> 
> > 
> > HTH

[Jeff Timanus]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > To back up a bit, the issue here is all about resource sharing between the WebGL context and the compositor context.  Different implementations of GraphicsContext3D may handle this in different ways, but in chromium today we do not support arbitrary resource sharing between these two.  prepareTexture() is wired up in chromium to do a texture copy (or multisample resolve) to a texture that is exposed to the compositor context.  In that sense, it's a form of service-side "magic" that allows a very limited form of resource sharing.  copyTextureToParentTextureCHROMIUM() was another form of service-side magic to support sharing (via a copy) a given texture with a texture in the compositor texture.  The approach was fundamentally broken, however, due to fact that it relied on a parent->child relationship between canvas contexts and compositor contexts, but that doesn't exist in reality.  A given WebGL <canvas> can migrate between different tabs, or between a window and a popup, which use different compositor contexts.
> > > 
> > > We could expose another entry point to allow sharing of a specific resource and a given compositor context resource.  For WebGL we only need this for framebuffer 0 so something like prepareTexture() makes sense.  I'm not completely sure what the best form of this API is, but whatever it is it shouldn't force us to do extra copies and it definitely cannot rely on a fixed parent-child relationship.
> > 
> > Thanks for the background, James.
> > 
> > I revisited the mechanics of the chromium swap-buffers implementation yesterday, hoping to see a clear way to push the contents from WebGL to the compositor, and I agree with your remarks on the service-side magic of swap-buffers, and the old copyTextureToParentTextureCHROMIUM.
> > 
> > Because I have been nursing this change for some time, I would like to get it in.  I would like to investigate and make a proposal for the sharing entry point that you suggest.  Has existing work already started on this?  Are there concerns relating to how the layer would be structured that haven't been touched upon already by this patch?  I don't want to tread on existing work that is already on-going.
> 
> Well this patch just straight up does not work in chromium today, WebGL and the compositor are not in a share group so you can't copy a texture to another.  We need a new entry point.  So no, we can not just put this in.

I think I didn't state my intent clearly enough.  By getting this change in, I mean that I would like to hold off on submitting it for the short term, so that we can implement the resource sharing mechanism.  After the mechanism is in place, hopefully only small modifications will be required to get this change up and running, at which point it can be submitted.

I'd prefer to not drop this change, if possible.

> 
> > 
> > On the other hand, I discussed this briefly with Vangelis, and he mentioned that we may want to hold off on making these changes until the threaded compositor is in place.  Can you comment on that suggestion?  Would the conversion to a threaded compositor have an impact on the design of a resource-sharing API/layer?
> 
> It has implications but I think they are solvable.  The place to start here is to figure out what API we need on GraphicsContext3D to enable resource sharing with the compositor.

I'll have a look at the existing mechanisms and submit a proposal in the next few days.

> 
> 
> > 
> > > 
> > > HTH

[Kenneth Russell]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > (In reply to comment #28)
> > > > To back up a bit, the issue here is all about resource sharing between the WebGL context and the compositor context.  Different implementations of GraphicsContext3D may handle this in different ways, but in chromium today we do not support arbitrary resource sharing between these two.  prepareTexture() is wired up in chromium to do a texture copy (or multisample resolve) to a texture that is exposed to the compositor context.  In that sense, it's a form of service-side "magic" that allows a very limited form of resource sharing.  copyTextureToParentTextureCHROMIUM() was another form of service-side magic to support sharing (via a copy) a given texture with a texture in the compositor texture.  The approach was fundamentally broken, however, due to fact that it relied on a parent->child relationship between canvas contexts and compositor contexts, but that doesn't exist in reality.  A given WebGL <canvas> can migrate between different tabs, or between a window and a popup, which use different compositor contexts.
> > > > 
> > > > We could expose another entry point to allow sharing of a specific resource and a given compositor context resource.  For WebGL we only need this for framebuffer 0 so something like prepareTexture() makes sense.  I'm not completely sure what the best form of this API is, but whatever it is it shouldn't force us to do extra copies and it definitely cannot rely on a fixed parent-child relationship.
> > > 
> > > Thanks for the background, James.
> > > 
> > > I revisited the mechanics of the chromium swap-buffers implementation yesterday, hoping to see a clear way to push the contents from WebGL to the compositor, and I agree with your remarks on the service-side magic of swap-buffers, and the old copyTextureToParentTextureCHROMIUM.
> > > 
> > > Because I have been nursing this change for some time, I would like to get it in.  I would like to investigate and make a proposal for the sharing entry point that you suggest.  Has existing work already started on this?  Are there concerns relating to how the layer would be structured that haven't been touched upon already by this patch?  I don't want to tread on existing work that is already on-going.
> > 
> > Well this patch just straight up does not work in chromium today, WebGL and the compositor are not in a share group so you can't copy a texture to another.  We need a new entry point.  So no, we can not just put this in.
> 
> I think I didn't state my intent clearly enough.  By getting this change in, I mean that I would like to hold off on submitting it for the short term, so that we can implement the resource sharing mechanism.  After the mechanism is in place, hopefully only small modifications will be required to get this change up and running, at which point it can be submitted.
> 
> I'd prefer to not drop this change, if possible.
> 
> > 
> > > 
> > > On the other hand, I discussed this briefly with Vangelis, and he mentioned that we may want to hold off on making these changes until the threaded compositor is in place.  Can you comment on that suggestion?  Would the conversion to a threaded compositor have an impact on the design of a resource-sharing API/layer?
> > 
> > It has implications but I think they are solvable.  The place to start here is to figure out what API we need on GraphicsContext3D to enable resource sharing with the compositor.
> 
> I'll have a look at the existing mechanisms and submit a proposal in the next few days.

CC'd apatrick on this bug as he has given this some thought. In an offline discussion with him, we should either provide a way to generate a framebuffer with the needed attachments in the compositor's context and map that to a client-side ID in the WebGL context, or bite the bullet and just share all resources client-side between WebGL and the compositor. The latter would require careful resource tracking to make sure that any OpenGL resource leaks in WebGL apps can't cause those resources to stick around for arbitrary periods of time in the compositor's context (but since they are sharing resources behind the scenes anyway, such bookkeeping is already in place and necessary).

I'd lean toward the latter solution as it would allow the DrawingBuffer class to allocate all of the objects it needs, with no magic going on behind the scenes.

> 
> > 
> > 
> > > 
> > > > 
> > > > HTH

[Chris Marrin]

I agree that all the work of dealing with buffers and window system specific interactions should be kept in DrawingBuffer. If DrawingBuffer has a prepareForCompositing (replacement for prepareTexture), then any platform specific work to get the buffer to the compositor should be able to be done there. 

This will allow us to keep GraphicsContext3D concerned with the GL-like API and avoid any window system dependencies

[Chris Marrin]

At the beginning of next week I'll have some cycles to devote to this issue and the restructuring of GC3D.

[Kenneth Russell]

twiz, to be clear, I'd hope that you weren't blocked making further progress on this patch. The idea is to use the new client-side resource sharing support in the command buffer code to put the WebGL context in the same share group as the compositor's context. Are you stuck?

[Jeff Timanus]

(In reply to comment #35)
> twiz, to be clear, I'd hope that you weren't blocked making further progress on this patch. The idea is to use the new client-side resource sharing support in the command buffer code to put the WebGL context in the same share group as the compositor's context. Are you stuck?

I'm not blocked per-se, but this change is blocked on the development of the resource sharing support mentioned above.  I'm looking into the requirements for implementing the resource sharing support.

Hopefully this patch wont bit-rot too much while that support is being built out.

[Jeff Timanus]

(In reply to comment #36)
> (In reply to comment #35)
> > twiz, to be clear, I'd hope that you weren't blocked making further progress on this patch. The idea is to use the new client-side resource sharing support in the command buffer code to put the WebGL context in the same share group as the compositor's context. Are you stuck?
> 
> I'm not blocked per-se, but this change is blocked on the development of the resource sharing support mentioned above.  I'm looking into the requirements for implementing the resource sharing support.
> 
> Hopefully this patch wont bit-rot too much while that support is being built out.

From a discussion with Al Patrick, he thinks that we should be safe to put the WebGL context in the same share group as the compositor, which should unblock this CL.  

I'll try this suggestion, and get another patch up (pending layout/conformance test success) in the next day.

Chris, this change is an extension of your previous patch on this issue.  Will we collide when you move these changes to the other ports?

[Chris Marrin]

(In reply to comment #37)
...
> Chris, this change is an extension of your previous patch on this issue.  Will we collide when you move these changes to the other ports?

I've decided to hold off on my changes to https://bugs.webkit.org/show_bug.cgi?id=66903 until after you've landed this. I really want all the buffer stuff out of GC3D. So I'm blocking my bug on this. Please let me know when you're done and we can proceed from there.

[Chris Marrin]

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > twiz, to be clear, I'd hope that you weren't blocked making further progress on this patch. The idea is to use the new client-side resource sharing support in the command buffer code to put the WebGL context in the same share group as the compositor's context. Are you stuck?
> > 
> > I'm not blocked per-se, but this change is blocked on the development of the resource sharing support mentioned above.  I'm looking into the requirements for implementing the resource sharing support.
> > 
> > Hopefully this patch wont bit-rot too much while that support is being built out.
> 
> From a discussion with Al Patrick, he thinks that we should be safe to put the WebGL context in the same share group as the compositor, which should unblock this CL.  
> 
> I'll try this suggestion, and get another patch up (pending layout/conformance test success) in the next day.
> 
> Chris, this change is an extension of your previous patch on this issue.  Will we collide when you move these changes to the other ports?

[Jeff Timanus]

(In reply to comment #38)
> (In reply to comment #37)
> ...
> > Chris, this change is an extension of your previous patch on this issue.  Will we collide when you move these changes to the other ports?
> 
> I've decided to hold off on my changes to https://bugs.webkit.org/show_bug.cgi?id=66903 until after you've landed this. I really want all the buffer stuff out of GC3D. So I'm blocking my bug on this. Please let me know when you're done and we can proceed from there.

From our discussion offline.  I'm working on reverting the changes of my patch that removed the platform layer handling from GraphicsContext3D.  Some of the recent changes around that code had rotted my change, and the removal of the  prepareTexture call is of higher priority.

Remaining unknown for this patch is testing the applicability of resource sharing between the compositor and the WebGL context.

Will post an update shortly.

[Jeff Timanus]

(In reply to comment #40)
> (In reply to comment #38)
> > (In reply to comment #37)
> > ...
> > > Chris, this change is an extension of your previous patch on this issue.  Will we collide when you move these changes to the other ports?
> > 
> > I've decided to hold off on my changes to https://bugs.webkit.org/show_bug.cgi?id=66903 until after you've landed this. I really want all the buffer stuff out of GC3D. So I'm blocking my bug on this. Please let me know when you're done and we can proceed from there.
> 
> From our discussion offline.  I'm working on reverting the changes of my patch that removed the platform layer handling from GraphicsContext3D.  Some of the recent changes around that code had rotted my change, and the removal of the  prepareTexture call is of higher priority.
> 
> Remaining unknown for this patch is testing the applicability of resource sharing between the compositor and the WebGL context.
> 
> Will post an update shortly.

An update on this work.  I've massaged my change so that it only introduces DrawingBuffer as the management entity for the FBO's used by the WebGLRenderingContext.

After much debugging, I've determined that there is a problem with the interaction of the WebGL context machinery and shared-resource contexts.  When resource sharing is enabled, the compositor's textures become black.  This happens both with my change, and without.

For example, from a clean Chrome checkout @99168, constructing the graphics context with shared resources (http://goo.gl/XnLrL) causes the compositor to only render black textures.  This is the same behaviour that I'm observing when integrating my DrawingBuffer changes.  My understanding is that making this single-line change should be a no-op.  Is this assumption correct?

I'm debugging this issue further.  Because the sharing works properly for Canvas2D, my gut tells me that this is a symptom of either:
  - A current context mis-match unique to the WebGL path.
  - A bug in the resource sharing code that the WebGL path is triggering.

For background, this entire change is blocked on this problematic behaviour.  The DrawingBuffer path requires resource sharing, as it is no longer using the parent-child 'prepareTexture' call.

[James Robinson]

There's special logic with WebGL contexts related to the preserveDrawingBuffer attribute (http://www.khronos.org/registry/webgl/specs/latest/#5.2.1), maybe that's biting you?  You could try different combinations of preserveDrawingBuffer/antialias to see if that makes any difference.

[Jeff Timanus]

(In reply to comment #42)
> There's special logic with WebGL contexts related to the preserveDrawingBuffer attribute (http://www.khronos.org/registry/webgl/specs/latest/#5.2.1), maybe that's biting you?  You could try different combinations of preserveDrawingBuffer/antialias to see if that makes any difference.

(In reply to comment #42)
> There's special logic with WebGL contexts related to the preserveDrawingBuffer attribute (http://www.khronos.org/registry/webgl/specs/latest/#5.2.1), maybe that's biting you?  You could try different combinations of preserveDrawingBuffer/antialias to see if that makes any difference.

Nope, the problem seems to be elsewhere.  All permutations of preserveDrawingBuffer, and antialias still exhibit the same problem, even on the no-op, single-line change.

[Jeff Timanus]

(In reply to comment #43)
> (In reply to comment #42)
> > There's special logic with WebGL contexts related to the preserveDrawingBuffer attribute (http://www.khronos.org/registry/webgl/specs/latest/#5.2.1), maybe that's biting you?  You could try different combinations of preserveDrawingBuffer/antialias to see if that makes any difference.
> 
> (In reply to comment #42)
> > There's special logic with WebGL contexts related to the preserveDrawingBuffer attribute (http://www.khronos.org/registry/webgl/specs/latest/#5.2.1), maybe that's biting you?  You could try different combinations of preserveDrawingBuffer/antialias to see if that makes any difference.
> 
> Nope, the problem seems to be elsewhere.  All permutations of preserveDrawingBuffer, and antialias still exhibit the same problem, even on the no-op, single-line change.

Update:  After much debugging, the problem is related to how extensions are handled between multiple contexts that are sharing resources.

Presently, WebGL contexts are created with the attribute noExtensions.  (http://goo.gl/FlJpj)  This turns into requesting the following extensions from GL: 
    "GL_OES_packed_depth_stencil "
    "GL_OES_depth24 "
    "GL_CHROMIUM_webglsl" (http://goo.gl/WGhCy)

The problem is that the compositor context is created after the WebGL context, and, via the resource sharing machinery, it re-uses the same set of validators that were constructed for the WebGL context.  Because the WebGL context did not request BGRA support, all of the TexSubImage2D calls for the compositor textures fail because they are BGRA.

The fix is to add BGRA support to the set of extensions requested by WebGL.

At a higher level, is this a critical problem for resource sharing?  Requiring that both contexts request the same set of extensions seems fragile, and limits the benefit of the explicit specification of requested extensions.

[Jeff Timanus]

Created attachment 109732 [details]
Patch

[Jeff Timanus]

(In reply to comment #45)
> Created an attachment (id=109732) [details]
> Patch

Finally an update on this dormant issue.  After lots of back-forth, I submitted a sister-change to Chrome that allowed resource sharing between WebGL and the compositor, which was required to make forward progress here.

See Chrome CL: http://codereview.chromium.org/7782038/

Patch (109732) implements a much simpler version of integrating DrawingBuffer into WebGLRenderingContext.  This patch also removes the texture copy that was performed in GraphicsContext3D::prepareTexture, by sharing the texture used as the colour buffer of the DrawingBuffer directly with the compositor.

[WebKit Review Bot]

Comment on attachment 109732 [details]
Patch

Attachment 109732 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9958132

[Early Warning System Bot]

Comment on attachment 109732 [details]
Patch

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

[John Bauman]

(In reply to comment #46)
> (In reply to comment #45)
> > Created an attachment (id=109732) [details] [details]
> > Patch
> 
> Finally an update on this dormant issue.  After lots of back-forth, I submitted a sister-change to Chrome that allowed resource sharing between WebGL and the compositor, which was required to make forward progress here.
> 
> See Chrome CL: http://codereview.chromium.org/7782038/
> 
> Patch (109732) implements a much simpler version of integrating DrawingBuffer into WebGLRenderingContext.  This patch also removes the texture copy that was performed in GraphicsContext3D::prepareTexture, by sharing the texture used as the colour buffer of the DrawingBuffer directly with the compositor.

There need to be two copies of the backing store when preserveDrawingBuffer=false (the default). I don't see how this patch accounts for that.

[Jeff Timanus]

(In reply to comment #49)
> (In reply to comment #46)
> > (In reply to comment #45)
> > > Created an attachment (id=109732) [details] [details] [details]
> > > Patch
> > 
> > Finally an update on this dormant issue.  After lots of back-forth, I submitted a sister-change to Chrome that allowed resource sharing between WebGL and the compositor, which was required to make forward progress here.
> > 
> > See Chrome CL: http://codereview.chromium.org/7782038/
> > 
> > Patch (109732) implements a much simpler version of integrating DrawingBuffer into WebGLRenderingContext.  This patch also removes the texture copy that was performed in GraphicsContext3D::prepareTexture, by sharing the texture used as the colour buffer of the DrawingBuffer directly with the compositor.
> 
> There need to be two copies of the backing store when preserveDrawingBuffer=false (the default). I don't see how this patch accounts for that.

Summary from an offline discussion:
The test that should catch regressions in preserveDrawingBuffer passes with these changes.

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/canvas/buffer-preserve-test.html

The test also passes when modified to explicitly disable multisampling.

On windows, several layout tests do fail with this change, but few of them appear directly related to WebGL:

Tests where results did not match expected results:
+media/video-playing-and-pause.html	expected actual diff wdiff 		TEXT	PASS

Flaky tests (failed the first run and got a different result on retry):
+compositing/geometry/object-clip-rects-assertion.html			TIMEOUT PASS	PASS

Tests that crashed:
+fast/canvas/webgl/gl-getshadersource.html	crash log

Tests that timed out:

Tests that had stderr output:
+fast/canvas/webgl/gl-getshadersource.html	stderr

Tests expected to fail but passed:
media/media-document-audio-repaint.html	IMAGE IMAGE+TEXT

[Jeff Timanus]

Created attachment 109737 [details]
Patch

[WebKit Review Bot]

Comment on attachment 109737 [details]
Patch

Attachment 109737 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9959048

[John Bauman]

Created attachment 109738 [details]
Modified webgl confomance test

This test dependence on the framework from the webgl conformance tests, so it needs to be copied over or next to the normal buffer-preserve-test.html in the webgl framework. Hopefully we could turn it into a real pixel test at some point.
The square should stay red. If it turns clear, that's a bug.

[Early Warning System Bot]

Comment on attachment 109737 [details]
Patch

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

[Jeff Timanus]

(In reply to comment #50)
> (In reply to comment #49)
> > (In reply to comment #46)
> > > (In reply to comment #45)
> > > > Created an attachment (id=109732) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > Finally an update on this dormant issue.  After lots of back-forth, I submitted a sister-change to Chrome that allowed resource sharing between WebGL and the compositor, which was required to make forward progress here.
> > > 
> > > See Chrome CL: http://codereview.chromium.org/7782038/
> > > 
> > > Patch (109732) implements a much simpler version of integrating DrawingBuffer into WebGLRenderingContext.  This patch also removes the texture copy that was performed in GraphicsContext3D::prepareTexture, by sharing the texture used as the colour buffer of the DrawingBuffer directly with the compositor.
> > 
> > There need to be two copies of the backing store when preserveDrawingBuffer=false (the default). I don't see how this patch accounts for that.
> 
> Summary from an offline discussion:
> The test that should catch regressions in preserveDrawingBuffer passes with these changes.
> 
> https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/canvas/buffer-preserve-test.html
> 
> The test also passes when modified to explicitly disable multisampling.
> 
> On windows, several layout tests do fail with this change, but few of them appear directly related to WebGL:
> 
> Tests where results did not match expected results:
> +media/video-playing-and-pause.html    expected actual diff wdiff         TEXT    PASS
> 
> Flaky tests (failed the first run and got a different result on retry):
> +compositing/geometry/object-clip-rects-assertion.html            TIMEOUT PASS    PASS
> 
> Tests that crashed:
> +fast/canvas/webgl/gl-getshadersource.html    crash log
> 
> Tests that timed out:
> 
> Tests that had stderr output:
> +fast/canvas/webgl/gl-getshadersource.html    stderr
> 
> Tests expected to fail but passed:
> media/media-document-audio-repaint.html    IMAGE IMAGE+TEXT

Further update:  This change regresses printing WebGL content.  Will investigate.

[Stephen White]

Comment on attachment 109737 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2956
> +        m_drawingBuffer->commit();

Since the commit()/do-something/bind() pattern seems to occur a few times, you could consider using a guard class.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4909
> +    m_drawingBuffer->resetOnNewContext(context);

If there's nothing to be preserved from the old DrawingBuffer, might it be a bit safer to just re-create the DrawingBuffer altogether here?  I suppose you might need a function on the DrawingBuffer to discard the old GPU resources without freeing them.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:319
> +        paintFramebufferToCanvas(drawingBuffer->framebuffer(), drawingBuffer->size().width(), drawingBuffer->size().height(),
> +                                 !m_impl->getContextAttributes().premultipliedAlpha, imageBuffer);
> +    } else
> +        paintFramebufferToCanvas(0, m_impl->width(), m_impl->height(), !m_impl->getContextAttributes().premultipliedAlpha, imageBuffer);

Could probably refactor this code (and paintRenderingResultsToImageData() below) into one call for both cases, by having width() and height() helper fns in this class that retrieved the appropriate size.  Up to you:  I'd only do it if it actually saved code.  :)

[Jeff Timanus]

Created attachment 110020 [details]
Patch

[Early Warning System Bot]

Comment on attachment 110020 [details]
Patch

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

[Jeff Timanus]

Created attachment 110555 [details]
Patch

[Jeff Timanus]

Comment on attachment 109737 [details]
Patch

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

Patch corrects order of construction of members in WebGLRenderingContext::WebGLRenderingContext.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2956
>> +        m_drawingBuffer->commit();
> 
> Since the commit()/do-something/bind() pattern seems to occur a few times, you could consider using a guard class.

I'd like to consider getting this patch in as is.  Following up with a subsequent patch implementing scope guards in a follow-up CL.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4909
>> +    m_drawingBuffer->resetOnNewContext(context);
> 
> If there's nothing to be preserved from the old DrawingBuffer, might it be a bit safer to just re-create the DrawingBuffer altogether here?  I suppose you might need a function on the DrawingBuffer to discard the old GPU resources without freeing them.

Done.  I removed DrawingBuffer::resetOnNewContext, and added DrawingBuffer::discardResources(), and construction of a new DrawingBuffer instance on context loss.

>> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:319
>> +        paintFramebufferToCanvas(0, m_impl->width(), m_impl->height(), !m_impl->getContextAttributes().premultipliedAlpha, imageBuffer);
> 
> Could probably refactor this code (and paintRenderingResultsToImageData() below) into one call for both cases, by having width() and height() helper fns in this class that retrieved the appropriate size.  Up to you:  I'd only do it if it actually saved code.  :)

Done.

[Jeff Timanus]

(In reply to comment #59)
> Created an attachment (id=110555) [details]
> Patch

This patch corrects the preserveDrawingBuffer behaviour, and does not regress printing of WebGL content.

PTAL.

[Early Warning System Bot]

Comment on attachment 110555 [details]
Patch

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

[Jeff Timanus]

Created attachment 110566 [details]
Patch

Attempt to correct Qt build issues.

[Early Warning System Bot]

Comment on attachment 110566 [details]
Patch

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

[Jeff Timanus]

Created attachment 110572 [details]
Patch

Attempt to correct Qt build issues.

[Early Warning System Bot]

Comment on attachment 110572 [details]
Patch

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

[James Robinson]

Comment on attachment 110572 [details]
Patch

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

I like the idea of keeping the compsitor's texture in an explicit separate copy, if that's what this patch is going for. It's a little hard for me to follow exactly what the responsibilities of DrawingBuffer vs WebGLRenderingContext vs compositor data structures are, could you describe at a higher level who's doing what?

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:66
> +    RefPtr<DrawingBuffer> drawingBuffer(DrawingBuffer::create(this, size, separateBackingBuffer));
> +    m_drawingBuffer = drawingBuffer.get();
> +    return drawingBuffer;

this appears to be creating a RefCounted object, taking a weak pointer to it, then passing ownership to the caller. Who's responsible for clearing the weak pointer when the DrawingBuffer is destroyed? The object ownership here seems confusing, at least.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:505
> +    DrawingBuffer* getDrawingBuffer() const { return m_drawingBuffer; }

we don't use "get" for getters in WebKit

> Source/WebCore/platform/graphics/GraphicsContext3D.h:957
> +    DrawingBuffer* m_drawingBuffer;
> +

what's the ownership model here?

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:122
> +        unsigned long colorFormat = m_context->getContextAttributes().alpha ? GraphicsContext3D::RGBA : GraphicsContext3D::RGB;

you should use either 'unsigned' or GC3Denum here, we don't use "unsigned long" in WebKit

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.h:92
> +    // Immediately releases ownership of all resources. Call upon loss of the
> +    // graphics context to prevent freeing invalid resources.
> +    void discardResources();

I don't understand the concern here - are you worried that we'll lose a context, restore it, and then issue delete calls for the old resource IDs?  Our command buffer implementation ASSERT()s if you issue deleteFoo() calls for 0, IIRC.

> Source/WebCore/platform/graphics/gpu/mac/DrawingBufferMac.mm:45
> +    :  m_separateBackingTexture(separateBackingTexture)
> +    , m_scissorEnabled(false)m_context(context)

something went horribly wrong here when merging it looks like

[Jeff Timanus]

Comment on attachment 110572 [details]
Patch

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

Thanks for your review, James.  I will address all of your comments in a forthcoming patch.

Here's a summary of what this change means:

My sister-change, (crrev.com/103724) prepared the command buffer back-end for resource sharing between WebGL and compositor contexts.  This change uses that to allow a DrawingBuffer instance to entirely manage the target rendering buffers for WebGL content.

Upon initialization, a WebGLRenderingContext will construct a DrawingBuffer instance. The DrawingBuffer will create up to three frame-buffer objects.  Two are optional:  If multi-sampling is requested, then multi-sample buffer is created.  If 'preserveDrawingBuffer' is false, then an optional backing store is created.  A non-multi-sampled buffer is always created.  

When the compositor requests the contents of the WebGL contents, it will read directly from the texture backing either the optional backing store, or the non-multi-sampled buffer of the DrawingBuffer.  If multisampling was requested, a multi-sample resolve will also happen at this time.

At a high level:
WebGLRenderingContext:
  - Constructs and owns the lifetime of the DrawingBuffer.
  - Assumes that DrawingBuffer replaces framebuffer-0 for all calls.
  - Has a reference to the GraphicsContext3D instance.

DrawingBuffer:
  - Manages the lifetime of all frame-buffer objects needed for the WebGL context.  (Multi-sample, non-multi-sample, backing store)
  - Provides the texture which the compositor uses to render WebGLContent.  (backing store, or non-multi-sample)
  - Has a reference to the GraphicsContext3D instance.

Compositor:
  - No real change.  Instead of using the texture returned from GraphicsContext3D::platformTexture for WebGL contents, it uses DrawingBuffer::platformColorBuffer().

>> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:66
>> +    return drawingBuffer;
> 
> this appears to be creating a RefCounted object, taking a weak pointer to it, then passing ownership to the caller. Who's responsible for clearing the weak pointer when the DrawingBuffer is destroyed? The object ownership here seems confusing, at least.

Yes, the ownership model is fairly confusing.  At present, the WebGLRenderingContext should be the hard owner of the DrawingBuffer.  

This is because DrawingBuffer already has a strong ref-counted pointer to the GraphicsContext3D that created it.  I did not want to add another strong pointer in GC3D back to the DrawingBuffer, and create a cycle.  

However, GraphicsContext3D does need access to the DrawingBuffer for the following calls:
  paintRenderingResultsToImageData, paintRenderingResultsToCanvas, and 
  WebGLLayerChromium::updateCompositorResources, and WebGLLayerChromium::setContext.

To remove need for the DrawingBuffer member the first two calls can be altered so that the caller passes the DrawingBuffer instance in.

The second calls are trickier to modify.  The GraphicsContext3D is the shared access point between the WebGLLayerChromium, and the WebGLRenderingContext.  If the WebGLRenderingContext could pass the DrawingBuffer instance to the WebGLLayerChromium directly, then there would be no need for the GraphicsContext3D to hold onto the DrawingBuffer instance.  

After doing some hunting, I think it's possible to re-structure some of the layer code so that the WebGLRenderingContext can directly pass in the DrawingBuffer instance at construction time.  

I'll submit a patch demonstrating this approach.

>> Source/WebCore/platform/graphics/GraphicsContext3D.h:505
>> +    DrawingBuffer* getDrawingBuffer() const { return m_drawingBuffer; }
> 
> we don't use "get" for getters in WebKit

Done.

>> Source/WebCore/platform/graphics/GraphicsContext3D.h:957
>> +
> 
> what's the ownership model here?

Mentioned in above note.  DrawingBuffer is owned by WebGLRenderingContext.  Weakly referred to here for access to framebuffer/width/height.

Also needed for access in WebGLLayerChromium.

Will remove in updated patch.

>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:122
>> +        unsigned long colorFormat = m_context->getContextAttributes().alpha ? GraphicsContext3D::RGBA : GraphicsContext3D::RGB;
> 
> you should use either 'unsigned' or GC3Denum here, we don't use "unsigned long" in WebKit

I followed the style from DrawingBuffer::reset, which also used unsigned long.  Corrected here, and in the old code.

>> Source/WebCore/platform/graphics/gpu/DrawingBuffer.h:92
>> +    void discardResources();
> 
> I don't understand the concern here - are you worried that we'll lose a context, restore it, and then issue delete calls for the old resource IDs?  Our command buffer implementation ASSERT()s if you issue deleteFoo() calls for 0, IIRC.

Yes, that is what I'm worried about.  The DrawingBuffer dtor invokes DrawingBuffer::clear(), which will only free the resources if they are non-null.

After this change, the behaviour of a lost context is to explicitly discard all of the resources on the DrawingBuffer, before assigning a new DrawingBuffer to the WebGLRenderingContext.  If the resources were not discarded, then non-0 ids would be freed in the old context, which is a potential error.  (Or am I wrong?) 

See WebGLRenderingContext::maybeRestore(...)

>> Source/WebCore/platform/graphics/gpu/mac/DrawingBufferMac.mm:45
>> +    , m_scissorEnabled(false)m_context(context)
> 
> something went horribly wrong here when merging it looks like

Fixed.

[Jeff Timanus]

Created attachment 110703 [details]
Patch

Remove GraphicsContext3D member variable.

[Jeff Timanus]

Comment on attachment 110703 [details]
Patch

This patch implements the changes suggested by jamesr.

I removed the raw DrawingBuffer pointer from GraphicsContext3D, but in order to do so, I had to expose LayerChromium to WebGLRenderingContext.  At initialization time, the WebGLRenderingContext pushes the DrawingBuffer to the PlatformLayer.

Also, can someone advise on how to fix the qt compile errors?  I believe that I need to change an include path for the other platform builds.

[Early Warning System Bot]

Comment on attachment 110703 [details]
Patch

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

[James Robinson]

(In reply to comment #70)
> Also, can someone advise on how to fix the qt compile errors?  I believe that I need to change an include path for the other platform builds.

I think you need to edit Source/WebCore/WebCore.pri to add this dir to the include path on Qt

[Jeff Timanus]

Created attachment 111188 [details]
Patch

[Early Warning System Bot]

Comment on attachment 111188 [details]
Patch

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

[John Bauman]

Comment on attachment 111188 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:616
> +        m_drawingBuffer->commit();

If a non-default framebuffer is bound, will that be restored after this set of operations?

[Jeff Timanus]

Comment on attachment 111188 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:616
>> +        m_drawingBuffer->commit();
> 
> If a non-default framebuffer is bound, will that be restored after this set of operations?

Good observation.

Done.

[Jeff Timanus]

Created attachment 111190 [details]
Patch

Address comments & attempt to fix EWS build.

[Jeff Timanus]

Comment on attachment 111190 [details]
Patch

PTAL.

I think we're finally getting very close!

[James Robinson]

Comment on attachment 111190 [details]
Patch

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

The relationship between DrawingBuffer, GraphicsContext3D, WebGLRenderingContext and the PlatformLayer still doesn't feel quite right.  With this setup it seems like responsibility for setting up the compositing layer is spread across too many different objects.  Would it make more sense for the DrawingBuffer itself own the PlatformLayer?  That way in Chromium DrawingBufferChromium can handle setting up the correct WebGLLayerChromium objects, on the Mac port DrawingBufferMac can do the right setup, etc.  WebGLRenderingContext wouldn't need to do any special setup.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:206
> +    void setDrawingBuffer(PassRefPtr<DrawingBuffer> drawingBuffer) { m_drawingBuffer = drawingBuffer; }
> +    PassRefPtr<DrawingBuffer> drawingBuffer() { return m_drawingBuffer; }
> +

this doesn't make any sense to have on LayerChromium - it's only relevant for WebGL layers. I think you need to either downcast at callsites (yuck) or rethink the object responsibilities.

[James Robinson]

(In reply to comment #78)
> (From update of attachment 111190 [details])
> PTAL.
> 
> I think we're finally getting very close!

By the way, I completely agree.  This is shaping up really nicely and I'm looking forward to seeing it completed!

[Jeff Timanus]

Created attachment 114144 [details]
Patch

[Early Warning System Bot]

Comment on attachment 114144 [details]
Patch

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

[Jeff Timanus]

Created attachment 114149 [details]
Patch

[Early Warning System Bot]

Comment on attachment 114149 [details]
Patch

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

[Jeff Timanus]

Created attachment 114166 [details]
Patch

[Early Warning System Bot]

Comment on attachment 114166 [details]
Patch

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

[Jeff Timanus]

Created attachment 114176 [details]
Patch

[Early Warning System Bot]

Comment on attachment 114176 [details]
Patch

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

[Jeff Timanus]

Created attachment 114180 [details]
Patch

[Early Warning System Bot]

Comment on attachment 114180 [details]
Patch

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

[Jeff Timanus]

Created attachment 114186 [details]
Patch

[Kenneth Russell]

Does that last patch need to be rebaselined?

I'd like to help you however I can to get this patch landed.

[Jeff Timanus]

Created attachment 114198 [details]
Patch

[Jeff Timanus]

Comment on attachment 114198 [details]
Patch

Ok.  One more time.  Where were we, again?

I re-consolidated the relationships between GraphicsContext3D, DrawingBuffer, WebGLRenderingContext, and WebGLLayerChromium.  The Drawing buffer is now the main point of interaction between all of these classes.

- GraphicsContext3D no longer caches the platform layer, and only references DrawingBuffer instances via arguments to the necessary calls.
- WebGLLayerChromium no longer keeps a reference to a GraphicsContext3D.  Instead, it keeps a raw pointer to the DrawingBuffer instance that created it.  A raw pointer is necessary to prevent a reference cycle.  The DrawingBuffer uninstalls itself from the WebGLLayerChromium at destruction.
- WebGLRenderingContext has no interaction with any of the platform layer code.  Only the DrawingBuffer is used to manage the FBO resources used by the context.

I've tested this change thoroughly.
 - Printing of WebGL content functions properly.
 - No layout test failures reported on Win7 with platform=chromium-gpu.
 - Of the webgl conformance tests, no new failures are introduced.  However, on my local machine, I do observe failures due to multi-sampling behaviour of my card (HD 5770).
  The following tests fail due to inperceptible pixel differences:
   conformance/glsl/functions/glsl-function-cross.html
   conformance/glsl/functions/glsl-function-dot.html
   conformance/glsl/functions/glsl-function-faceforward.html
   conformance/glsl/functions/glsl-function-reflect.html
   conformance/glsl/functions/glsl-function-smoothstep-float.html
   conformance/glsl/functions/glsl-function-smoothstep-gentype.html 
   
  The following tests are presently failing in the canary (17.0.932.0), and also fail with my change (these may also be related to the hardware AA of my GPU):
   conformance/glsl/functions/glsl-function-atan.html
   conformance/glsl/functions/glsl-function-atan-xy.html
   conformance/glsl/functions/glsl-function-mod-gentype.html
   conformance/glsl/misc/struct-nesting-under-maximum.html
   conformance/limits/gl-min-uniforms.html
   conformance/misc/object-deletion-behaviour.html
   conformance/renderbuffers/framebuffer-object-attachment.html
   conformance/textures/texture-mips.html

[Stephen White]

Great work, Jeff!

Since the mac EWS bot won't run for you, I'm applying this patch locally and building on Mac.

[Jeff Timanus]

(In reply to comment #95)
> Great work, Jeff!
> 
> Since the mac EWS bot won't run for you, I'm applying this patch locally and building on Mac.

From Stephen's build:  The mac version was failing due to warnings about unused arguments in GraphicsContext3DOpenGL.

Uploading a new patch addressing the problem.

[Stephen White]

(In reply to comment #96)
> (In reply to comment #95)
> > Great work, Jeff!
> > 
> > Since the mac EWS bot won't run for you, I'm applying this patch locally and building on Mac.
> 
> From Stephen's build:  The mac version was failing due to warnings about unused arguments in GraphicsContext3DOpenGL.
> 
> Uploading a new patch addressing the problem.

(In reply to comment #95)
> Great work, Jeff!
> 
> Since the mac EWS bot won't run for you, I'm applying this patch locally and building on Mac.

Since the (Safari) Mac build is currently broken, I had to sync to r99614 to get a working build to apply against.

It gave the following warnings-treated-as-errors:

/Volumes/MacintoshHD2/senorblanco/src/webkit1/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:131: warning: unused parameter ‘drawingBuffer’
/Volumes/MacintoshHD2/senorblanco/src/webkit1/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:167: warning: unused parameter ‘drawingBuffer’
PhaseScriptExecution "Check For Inappropriate Files In Framework" /Volumes/MacintoshHD2/senorblanco/src/webkit1/WebKitBuild/WebCore.build/Release/WebCore.build/Script-5DF50887116F3077005202AB.sh

Removing the parameter and the ASSERT below it allowed it to compile.

Running new-run-webkit-tests, I got 11 unexpected failures both with and without your patch.

[Jeff Timanus]

Created attachment 114312 [details]
Patch

[Jeff Timanus]

(In reply to comment #98)
> Created an attachment (id=114312) [details]
> Patch

Patch builds successfully @ Webkit rev 99614.

37 layout failures are reported on mac, none of which are related to webgl:

Regressions: Unexpected text diff mismatch : (17)
  animations/animation-direction-normal.html = TEXT
  compositing/reflections/animation-inside-reflection.html = TEXT
  compositing/tiling/crash-reparent-tiled-layer.html = TEXT
  css3/selectors3/xml/css3-modsel-d2.xml = TEXT
  fast/canvas/canvas-composite-canvas.html = TEXT
  fast/canvas/canvas-composite-image.html = TEXT
  fast/css/custom-font-xheight.html = TEXT
  fast/css/font-face-multiple-faces.html = TEXT
  fast/dom/HTMLScriptElement/script-set-src.html = TEXT
  fast/writing-mode/japanese-rl-text-with-broken-font.html = TEXT
  http/tests/inspector/resource-tree/resource-tree-errors-reload.html = TEXT
  http/tests/inspector/resource-tree/resource-tree-non-unique-url.html = TEXT
  http/tests/local/style-access-before-stylesheet-loaded.html = TEXT
  inspector/debugger/debugger-scripts.html = TEXT
  inspector/styles/commit-selector.html = TEXT
  inspector/timeline/timeline-receive-response-event.html = TEXT
  java/lc3/ArrayMethods/byte-001.html = TEXT

Regressions: Unexpected tests timed out : (20)
  fast/css/visited-link-hang.html = TIMEOUT
  fast/dom/NodeList/adoptNode-node-list-cache.html = TIMEOUT
  fast/dom/gc-9.html = TIMEOUT
  fast/dom/object-plugin-hides-properties.html = TIMEOUT
  fast/js/nested-object-gc.html = TIMEOUT
  java/lc3/CallStatic/boolean-001.html = TIMEOUT
  java/lc3/ConvertJSObject/ToBoolean-001.html = TIMEOUT
  java/lc3/ConvertNull/null-001.html = TIMEOUT
  java/lc3/ConvertNumber/number-001.html = TIMEOUT
  java/lc3/ConvertString/string-001.html = TIMEOUT
  java/lc3/ConvertUndefined/undefined-001-n.html = TIMEOUT
  java/lc3/Exceptions/throw_js_types.html = TIMEOUT
  java/lc3/JSBoolean/boolean-001.html = TIMEOUT
  java/lc3/JSNull/ToBoolean-001-n.html = TIMEOUT
  java/lc3/JSNumber/ToByte-001.html = TIMEOUT
  java/lc3/JSObject/ToByte-001.html = TIMEOUT
  java/lc3/JSUndefined/undefined-001.html = TIMEOUT
  jquery/attributes.html = TIMEOUT
  media/adopt-node-crash.html = TIMEOUT
  platform/mac/accessibility/math-alttext.html = TIMEOUT

[Stephen White]

Comment on attachment 114312 [details]
Patch

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

Looks good to me, but I'll leave for other reviewers.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:607
>          m_context->paintCompositedResultsToCanvas(this);

As discussed, please check if this call can be removed in a followup patch (if it's now a no-op on all platforms).   If not, at least for Chromium it should probably be in an #else of the #ifdef below.  It's a little confusing to see calls to both functions if only one is necessary.

[James Robinson]

Comment on attachment 114312 [details]
Patch

R=me, looks great.  Not touching cq? - please flip that at your convenience (or ask Stephen if you aren't a committer) and monitor the bots carefully when this lands.

[WebKit Review Bot]

Comment on attachment 114312 [details]
Patch

Rejecting attachment 114312 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [GTK] Make the ENABLE(FEATURE) macro work in DRT

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 157.

Full output: http://queues.webkit.org/results/10400338

[Jeff Timanus]

Created attachment 114511 [details]
Patch

[Jeff Timanus]

Comment on attachment 114511 [details]
Patch

Trying this again?  Commit queue, please?

(I updated the change locally, and observed no conflicts on the ChangeLog files.)

[Stephen White]

Comment on attachment 114511 [details]
Patch

Done.  r=me

[WebKit Review Bot]

Comment on attachment 114511 [details]
Patch

Clearing flags on attachment: 114511

Committed r100032: <http://trac.webkit.org/changeset/100032>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

This is fantastic. Jeff, thanks for pushing this through.