Bug 36248

Summary: Implement lazy clearing of renderbuffers
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, kbr, oliver, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 126538    
Attachments:
Description Flags
patch
none
revised patch : responding to kbr's review
dglazkov: review-, dglazkov: commit-queue-
revised patch : responding to Dimitri Glazkov's review none

Description Kenneth Russell 2010-03-17 16:00:11 PDT
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.
Comment 1 Zhenyao Mo 2010-05-20 20:09:28 PDT
Created attachment 56665 [details]
patch
Comment 2 Kenneth Russell 2010-05-24 14:24:24 PDT
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.
Comment 3 Zhenyao Mo 2010-05-24 14:45:27 PDT
(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.
Comment 4 Kenneth Russell 2010-05-24 14:48:40 PDT
(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.
Comment 5 Zhenyao Mo 2010-05-24 15:02:08 PDT
(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.
Comment 6 Kenneth Russell 2010-05-24 15:46:51 PDT
(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?
Comment 7 Zhenyao Mo 2010-05-24 15:51:20 PDT
(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.
Comment 8 Zhenyao Mo 2010-05-24 16:39:26 PDT
Created attachment 56944 [details]
revised patch : responding to kbr's review
Comment 9 Kenneth Russell 2010-05-24 17:05:49 PDT
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.
Comment 10 Dimitri Glazkov (Google) 2010-05-25 09:49:03 PDT
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?
Comment 11 Zhenyao Mo 2010-05-25 10:57:01 PDT
(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.
Comment 12 Zhenyao Mo 2010-05-25 10:58:37 PDT
Created attachment 57029 [details]
revised patch : responding to Dimitri Glazkov's review
Comment 13 Dimitri Glazkov (Google) 2010-05-25 13:54:31 PDT
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.
Comment 14 WebKit Commit Bot 2010-05-27 04:43:47 PDT
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>
Comment 15 WebKit Commit Bot 2010-05-27 04:43:52 PDT
All reviewed patches have been landed.  Closing bug.