Bug 36248 – Implement lazy clearing of renderbuffers

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

[Kenneth Russell]

Per the section of the WebGL spec on Resource Restrictions (currently Section 4.2), the contents of renderbuffers must be initialized before their first use, so that garbage can not be read back from them. To implement this, when a renderbuffer is first attached to a complete FBO, or when an FBO becomes complete and it has uninitialized renderbuffers attached, the appropriate planes of the FBO must be cleared.

[Zhenyao Mo]

Created attachment 56665 [details]
patch

[Kenneth Russell]

Comment on attachment 56665 [details]
patch

This looks good overall. A few comments, one affecting correctness.

WebCore/html/canvas/WebGLFramebuffer.cpp:83
 +      // lifespan of CanvanObject changes.
CanvanObject -> CanvasObject


WebCore/html/canvas/WebGLFramebuffer.cpp:196
 +      }
This can be written more concisely:

if (initDepth && initStencil && m_depthStencilAttachment) {
    (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized();
} else {
    if (initDepth)
        (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized();
    if (initStencil)
        (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized();
}


WebCore/html/canvas/WebGLFramebuffer.h:73
 +          CanvasObject* m_depthStencilAttachment;
These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers.

[Zhenyao Mo]

(In reply to comment #2)
> (From update of attachment 56665 [details])
> This looks good overall. A few comments, one affecting correctness.
> 
> WebCore/html/canvas/WebGLFramebuffer.cpp:83
>  +      // lifespan of CanvanObject changes.
> CanvanObject -> CanvasObject
> 
> 
> WebCore/html/canvas/WebGLFramebuffer.cpp:196
>  +      }
> This can be written more concisely:
> 
> if (initDepth && initStencil && m_depthStencilAttachment) {
>     (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized();
> } else {
>     if (initDepth)
>         (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized();
>     if (initStencil)
>         (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized();
> }
> 
> 
> WebCore/html/canvas/WebGLFramebuffer.h:73
>  +          CanvasObject* m_depthStencilAttachment;
> These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers.

In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects.

[Kenneth Russell]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 56665 [details] [details])
> > This looks good overall. A few comments, one affecting correctness.
> > 
> > WebCore/html/canvas/WebGLFramebuffer.cpp:83
> >  +      // lifespan of CanvanObject changes.
> > CanvanObject -> CanvasObject
> > 
> > 
> > WebCore/html/canvas/WebGLFramebuffer.cpp:196
> >  +      }
> > This can be written more concisely:
> > 
> > if (initDepth && initStencil && m_depthStencilAttachment) {
> >     (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized();
> > } else {
> >     if (initDepth)
> >         (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized();
> >     if (initStencil)
> >         (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized();
> > }
> > 
> > 
> > WebCore/html/canvas/WebGLFramebuffer.h:73
> >  +          CanvasObject* m_depthStencilAttachment;
> > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers.
> 
> In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects.

I don't believe this is sufficient. Consider this sequence of operations:

1. A renderbuffer is attached to a framebuffer. A pointer to the WebGLRenderbuffer is stored in the WebGLFramebuffer object. For the purposes of example, say that the framebuffer is still incomplete at this point.
2. All JavaScript references to the renderbuffer, but not the framebuffer, are dropped. The WebGLRenderbuffer's reference count goes to 0 and the C++ object is deleted. The WebGLFramebuffer now points to deleted storage.
3. A different renderbuffer is attached to the WebGLFramebuffer. The old, deleted pointer, is queried.

[Zhenyao Mo]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 56665 [details] [details] [details])
> > > This looks good overall. A few comments, one affecting correctness.
> > > 
> > > WebCore/html/canvas/WebGLFramebuffer.cpp:83
> > >  +      // lifespan of CanvanObject changes.
> > > CanvanObject -> CanvasObject
> > > 
> > > 
> > > WebCore/html/canvas/WebGLFramebuffer.cpp:196
> > >  +      }
> > > This can be written more concisely:
> > > 
> > > if (initDepth && initStencil && m_depthStencilAttachment) {
> > >     (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized();
> > > } else {
> > >     if (initDepth)
> > >         (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized();
> > >     if (initStencil)
> > >         (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized();
> > > }
> > > 
> > > 
> > > WebCore/html/canvas/WebGLFramebuffer.h:73
> > >  +          CanvasObject* m_depthStencilAttachment;
> > > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers.
> > 
> > In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects.
> 
> I don't believe this is sufficient. Consider this sequence of operations:
> 
> 1. A renderbuffer is attached to a framebuffer. A pointer to the WebGLRenderbuffer is stored in the WebGLFramebuffer object. For the purposes of example, say that the framebuffer is still incomplete at this point.
> 2. All JavaScript references to the renderbuffer, but not the framebuffer, are dropped. The WebGLRenderbuffer's reference count goes to 0 and the C++ object is deleted. The WebGLFramebuffer now points to deleted storage.
> 3. A different renderbuffer is attached to the WebGLFramebuffer. The old, deleted pointer, is queried.

That's correct.  But once an CanvasObject is created, a RefPtr is added in the object list in WebGLRenderingContext, and is never deleted until the destruction of WebGLRenderingContext.  So in this case, we don't need to worry about the renference count goes to 0.

[Kenneth Russell]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (From update of attachment 56665 [details] [details] [details] [details])
> > > > This looks good overall. A few comments, one affecting correctness.
> > > > 
> > > > WebCore/html/canvas/WebGLFramebuffer.cpp:83
> > > >  +      // lifespan of CanvanObject changes.
> > > > CanvanObject -> CanvasObject
> > > > 
> > > > 
> > > > WebCore/html/canvas/WebGLFramebuffer.cpp:196
> > > >  +      }
> > > > This can be written more concisely:
> > > > 
> > > > if (initDepth && initStencil && m_depthStencilAttachment) {
> > > >     (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized();
> > > > } else {
> > > >     if (initDepth)
> > > >         (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized();
> > > >     if (initStencil)
> > > >         (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized();
> > > > }
> > > > 
> > > > 
> > > > WebCore/html/canvas/WebGLFramebuffer.h:73
> > > >  +          CanvasObject* m_depthStencilAttachment;
> > > > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers.
> > > 
> > > In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects.
> > 
> > I don't believe this is sufficient. Consider this sequence of operations:
> > 
> > 1. A renderbuffer is attached to a framebuffer. A pointer to the WebGLRenderbuffer is stored in the WebGLFramebuffer object. For the purposes of example, say that the framebuffer is still incomplete at this point.
> > 2. All JavaScript references to the renderbuffer, but not the framebuffer, are dropped. The WebGLRenderbuffer's reference count goes to 0 and the C++ object is deleted. The WebGLFramebuffer now points to deleted storage.
> > 3. A different renderbuffer is attached to the WebGLFramebuffer. The old, deleted pointer, is queried.
> 
> That's correct.  But once an CanvasObject is created, a RefPtr is added in the object list in WebGLRenderingContext, and is never deleted until the destruction of WebGLRenderingContext.  So in this case, we don't need to worry about the renference count goes to 0.

Right, I'd forgotten about that object list. Could you instead add a comment above these fields in WebGLFramebuffer indicating that these objects are kept alive by the global table in WebGLRenderingContext?

[Zhenyao Mo]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > (From update of attachment 56665 [details] [details] [details] [details] [details])
> > > > > This looks good overall. A few comments, one affecting correctness.
> > > > > 
> > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:83
> > > > >  +      // lifespan of CanvanObject changes.
> > > > > CanvanObject -> CanvasObject
> > > > > 
> > > > > 
> > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:196
> > > > >  +      }
> > > > > This can be written more concisely:
> > > > > 
> > > > > if (initDepth && initStencil && m_depthStencilAttachment) {
> > > > >     (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized();
> > > > > } else {
> > > > >     if (initDepth)
> > > > >         (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized();
> > > > >     if (initStencil)
> > > > >         (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized();
> > > > > }
> > > > > 
> > > > > 
> > > > > WebCore/html/canvas/WebGLFramebuffer.h:73
> > > > >  +          CanvasObject* m_depthStencilAttachment;
> > > > > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers.
> > > > 
> > > > In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects.
> > > 
> > > I don't believe this is sufficient. Consider this sequence of operations:
> > > 
> > > 1. A renderbuffer is attached to a framebuffer. A pointer to the WebGLRenderbuffer is stored in the WebGLFramebuffer object. For the purposes of example, say that the framebuffer is still incomplete at this point.
> > > 2. All JavaScript references to the renderbuffer, but not the framebuffer, are dropped. The WebGLRenderbuffer's reference count goes to 0 and the C++ object is deleted. The WebGLFramebuffer now points to deleted storage.
> > > 3. A different renderbuffer is attached to the WebGLFramebuffer. The old, deleted pointer, is queried.
> > 
> > That's correct.  But once an CanvasObject is created, a RefPtr is added in the object list in WebGLRenderingContext, and is never deleted until the destruction of WebGLRenderingContext.  So in this case, we don't need to worry about the renference count goes to 0.
> 
> Right, I'd forgotten about that object list. Could you instead add a comment above these fields in WebGLFramebuffer indicating that these objects are kept alive by the global table in WebGLRenderingContext?

Will do.

[Zhenyao Mo]

Created attachment 56944 [details]
revised patch : responding to kbr's review

[Kenneth Russell]

Comment on attachment 56944 [details]
revised patch : responding to kbr's review

Looks good to me. Nice work, in particular on handling the case where a color attachment becomes valid because of a call to texImage2D or copyTexImage2D.

[Dimitri Glazkov (Google)]

Comment on attachment 56944 [details]
revised patch : responding to kbr's review

Just a few typos:

WebCore/ChangeLog:5
 +          Implement lazy clearing of renderbuffers
Typo: implement.

WebCore/html/canvas/WebGLFramebuffer.cpp:134
 +      bool isSissorEnabled = false;
Typo: Scissor

WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp:332
 +      GLboolean isSissorEnabled = GL_FALSE;
Ditto.


Another thing -- this is just a clarity suggestion. Would it be more descriptive to define a simple Attachment abstraction, which holds CanvasObject* and manages all casting and validity checking?

[Zhenyao Mo]

(In reply to comment #10)
> (From update of attachment 56944 [details])
> Just a few typos:
> 
> WebCore/ChangeLog:5
>  +          Implement lazy clearing of renderbuffers
> Typo: implement.
> 

The original spelling is correct.

> WebCore/html/canvas/WebGLFramebuffer.cpp:134
>  +      bool isSissorEnabled = false;
> Typo: Scissor
> 

Fixed.

> WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp:332
>  +      GLboolean isSissorEnabled = GL_FALSE;
> Ditto.
> 

Fixed.

> 
> Another thing -- this is just a clarity suggestion. Would it be more descriptive to define a simple Attachment abstraction, which holds CanvasObject* and manages all casting and validity checking?

I added two helper functions to deal with casting and validity checking.  Now the code is much cleaner.  Thanks for the suggestion.

[Zhenyao Mo]

Created attachment 57029 [details]
revised patch : responding to Dimitri Glazkov's review

[Dimitri Glazkov (Google)]

Comment on attachment 57029 [details]
revised patch : responding to Dimitri Glazkov's review

ok. isInitialized/isUninitialized could've been just local functions, but this works too.

[WebKit Commit Bot]

Comment on attachment 57029 [details]
revised patch : responding to Dimitri Glazkov's review

Clearing flags on attachment: 57029

Committed r60290: <http://trac.webkit.org/changeset/60290>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.