WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
63635
Improve WebGL object lifetime management in WebGLRenderingContext
https://bugs.webkit.org/show_bug.cgi?id=63635
Summary
Improve WebGL object lifetime management in WebGLRenderingContext
Zhenyao Mo
Reported
2011-06-29 10:15:38 PDT
At the moment, all objects are held by WebGLRenderingContext even after the underlying gl object is explicitly deleted. It seems like we don't need to keep this object table (m_canvasObjects) in WebGLRenderingContext. For all the objects that can be queried (for example, current program), we already hold them besides this object table (for example, m_currentProgram). Currently the only usage of this object table is that when we query an object, we get an integer from underlying GL, and we go through this table to find the corresponding WebGL object. If we remove this table, we can just return the held WebGL object instead of querying the underlying GL and then find the WebGLObject matching it.
Attachments
Patch
(22.04 KB, patch)
2011-06-30 09:56 PDT
,
Zhenyao Mo
no flags
Details
Formatted Diff
Diff
Patch
(22.97 KB, patch)
2011-06-30 15:35 PDT
,
Zhenyao Mo
kbr
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Zhenyao Mo
Comment 1
2011-06-30 09:56:24 PDT
Created
attachment 99321
[details]
Patch
Zhenyao Mo
Comment 2
2011-06-30 09:57:20 PDT
The tests are fixed on the khronos and synced here. Also, I manually verified that now garbage collection does collect the WebGL objects that are no longer referenced. Please review.
Kenneth Russell
Comment 3
2011-06-30 14:49:54 PDT
Comment on
attachment 99321
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=99321&action=review
Nice work; generally looks good except for one cleanup issue in detachAndRemoveAllObjects.
> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2033 > + m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM);
This is correct, but it might be worth a note here that OpenGL ES 2.0 specifies INVALID_ENUM in this case, while desktop GL specifies INVALID_OPERATION.
> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2043 > + return WebGLGetInfo(PassRefPtr<WebGLTexture>(reinterpret_cast<WebGLTexture*>(object)));
I think you should be using adoptRef rather than calling the PassRefPtr constructor explicitly here.
> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2061 > + return WebGLGetInfo(PassRefPtr<WebGLRenderbuffer>(reinterpret_cast<WebGLRenderbuffer*>(object)));
Same thing about using adoptRef here.
> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4001 > }
This looks very complicated and flaky. If you're worried about WebGLObject::detachContext() removing the object from the table and changing the iteration order, copy the HashSet<WebGLObject*> into a Vector<WebGLObject*> and iterate down the vector. Also, you might want to consider having WebGLObject::detachContext() remove the object from this table rather than the destructor of WebGLObject. Right now calling detachContext() on the WebGLObject prevents the WebGLObject from removing itself from this table.
Zhenyao Mo
Comment 4
2011-06-30 15:31:33 PDT
(In reply to
comment #3
)
> (From update of
attachment 99321
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=99321&action=review
> > Nice work; generally looks good except for one cleanup issue in detachAndRemoveAllObjects. > > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2033 > > + m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM); > > This is correct, but it might be worth a note here that OpenGL ES 2.0 specifies INVALID_ENUM in this case, while desktop GL specifies INVALID_OPERATION.
Done.
> > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2043 > > + return WebGLGetInfo(PassRefPtr<WebGLTexture>(reinterpret_cast<WebGLTexture*>(object))); > > I think you should be using adoptRef rather than calling the PassRefPtr constructor explicitly here.
There is a difference between adoptRef and explicit constructor. Explicit constructor will call ref() if the pointer is non-null. If we use adoptRef here, several tests crash.
> > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2061 > > + return WebGLGetInfo(PassRefPtr<WebGLRenderbuffer>(reinterpret_cast<WebGLRenderbuffer*>(object))); > > Same thing about using adoptRef here. > > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4001 > > } > > This looks very complicated and flaky. If you're worried about WebGLObject::detachContext() removing the object from the table and changing the iteration order, copy the HashSet<WebGLObject*> into a Vector<WebGLObject*> and iterate down the vector. > > Also, you might want to consider having WebGLObject::detachContext() remove the object from this table rather than the destructor of WebGLObject. Right now calling detachContext() on the WebGLObject prevents the WebGLObject from removing itself from this table.
Done.
Zhenyao Mo
Comment 5
2011-06-30 15:35:22 PDT
Created
attachment 99378
[details]
Patch
Kenneth Russell
Comment 6
2011-06-30 15:58:11 PDT
Comment on
attachment 99378
[details]
Patch Looks better to me. Nice work.
Zhenyao Mo
Comment 7
2011-06-30 16:26:49 PDT
Committed
r90180
: <
http://trac.webkit.org/changeset/90180
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug