572283 - Fix CanvasLayerOGL issues with retained layers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

CanvasLayerOGL is a bit broken on Mac with retained layers:
-- some work that's done on every paint should be moved to Updated and the destructor
-- We were passing the wrong CGLContext to CGLTexImagePBuffer (we need to pass the destination context, not the pbuffer context)
-- Bogus assertion in LayerManagerOGL; it's OK to not have a root at the start of a transaction

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Retain CGLContext for the lifetime of its widget.

We can crash if the CGLContext is released before the widget.

Also, GetLayerManager should not crash if we have no outer window. This can happen temporarily as we go fullscreen, apparently.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, so maybe I'm not undertanding something, when retained layers are in use -- the problem is calling BindTexImage makes rendering to the bound buffer undefined.  So it's not something that can just be done once, it has to be done just when the pbuffer is to be used as a texture, and it has to be detached each time.  This is why it was done during Render time, and not during Updated() time.  Without retained layers, we create a new layer each time and just call Updated() right away, but that's being called while we're actually rendering and building up the layer tree.  When does Updated() get called with retained layers?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 451467 [details] [diff] [review]
Part 2: Retain CGLContext for the lifetime of its widget.

we may want to rename this to mCGLContext; mContext is fairly generic

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

WebGLContext::GetCanvasLayer and nsCanvasRenderingContext2D::GetCanvasLayer call Updated when they returns an old layer.

I can modify the patch to Unbind after RenderLayer and Bind on every Updated call, if you like.

I don't think we need to do that on Mac, though. See
http://developer.apple.com/mac/library/documentation/GraphicsImaging/Reference/CGL_OpenGL/Reference/reference.html#//apple_ref/c/func/CGLTexImagePBuffer
If you modify the content of a pixel buffer that uses mipmap levels, you must call this function again before drawing with the pixel buffer, to ensure that the content is synchronized with OpenGL. For pixel buffers without mipmaps, simply rebind to the texture object to synchronize content.

Sound OK?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

As long as we can ensure that no script will execute between GetCanvasLayer/RenderLayer, that sounds fine.  On Win32, the mac bit isn't true -- from the WGL_render_texture extension:

    -   The application must release the color buffer from the texture 
        before it can render to the pbuffer again. When the color buffer 
        is bound as a texture, draw and read operations on the pbuffer 
        are undefined. 

I guess we can skip it on OSX.  (Note that I believe these two statements are using the term 'bind' differently.)  One additional interesting bit:

11. When the color buffer is released from the texture (back to the pbuffer)
    should the contents be preserved? 

    No, this may prove difficult to implement on some architectures. 

That's interesting, because that's not how things work currently on any win32 box that I've tried.  But it sounds like this is all the more reason to start using FBOs for this.  I'll work on that at some point soon.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1 v2

Fixed to rebind Windows pbuffer at every paint.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: fix GL compiler warnings

Trivial

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 451814 [details] [diff] [review]
Part 1 v2


>+#if defined(XP_MACOSX)
>+    // We only need to do this for the first time we set up the texture
>+    if (mTexture == 0) {

This needs to be "if (newTexture)", right?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Er, yep!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1 v3

Addressed comment.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/b5b2dd87d528
http://hg.mozilla.org/mozilla-central/rev/5f9932cbe799
http://hg.mozilla.org/mozilla-central/rev/22cffed4ebda