Bug 75906

Summary: Add WebGLContextGroup as step toward sharing WebGL resources
Product: WebKit Reporter: Gregg Tavares <gman>
Component: WebGLAssignee: Gregg Tavares <gman>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, kbr, rakuco, webkit.review.bot, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Gregg Tavares 2012-01-09 16:24:42 PST 
Add WebGLContextGroup as step toward sharing WebGL resources
Comment 1 Gregg Tavares 2012-01-09 17:21:44 PST 
Created attachment 121765 [details]
Patch
Comment 2 Gregg Tavares 2012-01-09 17:23:25 PST 
This patch adds WebGLContextGroup. There are NO API CHANGES

This addition makes it possible to implement the upcoming WebGL
sharing resources spec being worked on by the WebGL Working Group.
Comment 3 Kenneth Russell 2012-01-09 17:34:34 PST 
Looks like the patch needs to be rebaselined to TOT. Could you do that and resubmit?
Comment 4 Gregg Tavares 2012-01-10 12:48:11 PST 
Created attachment 121894 [details]
Patch
Comment 5 Early Warning System Bot 2012-01-10 12:57:16 PST 
Comment on attachment 121894 [details]
Patch

Attachment 121894 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11167562
Comment 6 Gregg Tavares 2012-01-10 17:01:26 PST 
Created attachment 121944 [details]
Patch
Comment 7 Gregg Tavares 2012-01-10 18:20:45 PST 
Created attachment 121956 [details]
Patch
Comment 8 Kenneth Russell 2012-01-10 19:55:48 PST 
Comment on attachment 121956 [details]
Patch

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

Good refactoring overall. Minor comments and questions only. If all of my comments are invalid and you think this patch should proceed as is, go ahead and mark r? again.

> Source/WebCore/GNUmakefile.list.am:548
> +	DerivedSources/WebCore/JSWebGLContextGroup.h \

WebGLContextGroup doesn't have any IDL -- so these shouldn't be necessary (and their presence should, I think, cause build failures).

> Source/WebCore/html/canvas/WebGLContextGroup.cpp:74
> +    if (m_contexts.isEmpty()) {

How about writing this as a pre-check: if (m_contexts.size() == 1 && m_contexts.contains(context)) { detachAndRemvoeAllObjects(); } thereby avoiding the re-adding and removal of the context.

> Source/WebCore/html/canvas/WebGLContextGroup.cpp:89
> +    removeObject(object);

Why is this call necessary? The are two other similar calls in WebGLRenderingContext.cpp, addSharedObject() and addContextObject().

> Source/WebCore/html/canvas/WebGLContextObject.h:37
> +class WebGLContextObject : public WebGLObject {

This class definitely needs a comment indicating that it represents WebGL resources that are per-context, rather than shared among contexts.

> Source/WebCore/html/canvas/WebGLContextObject.h:67
> +#endif // WebGLObject_h

Wrong #include guard.

> Source/WebCore/html/canvas/WebGLFramebuffer.h:46
> +    void setAttachmentForBoundFramebuffer(GraphicsContext3D*, GC3Denum attachment, GC3Denum texTarget, WebGLTexture*, GC3Dint level);

Since WebGLFramebuffer is a per-context object, it's unnecessary to pass in the GraphicsContext3D* here, as well as in removeAttachmentFromBoundFramebuffer and onAccess. I think the changes to those methods, and the calling code in WebGLRenderingContext, should be reverted in order to simplify the patch.

> Source/WebCore/html/canvas/WebGLFramebuffer.h:-87
> -    bool isBound() const;

Why remove this? It seems that the associated assertions would still be correct, since WebGLFramebuffer is a per-context object.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4186
> +    removeSharedObject(object);

Per comment above, I think this line should be removed.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4198
> +    removeContextObject(object);

Per comment above, I think this line should be removed.
Comment 9 Gregg Tavares 2012-01-12 13:28:43 PST 
Created attachment 122297 [details]
Patch
Comment 10 Gregg Tavares 2012-01-12 13:29:29 PST 
Comment on attachment 121956 [details]
Patch

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

>> Source/WebCore/GNUmakefile.list.am:548
>> +	DerivedSources/WebCore/JSWebGLContextGroup.h \
> 
> WebGLContextGroup doesn't have any IDL -- so these shouldn't be necessary (and their presence should, I think, cause build failures).

done

>> Source/WebCore/html/canvas/WebGLContextGroup.cpp:74
>> +    if (m_contexts.isEmpty()) {
> 
> How about writing this as a pre-check: if (m_contexts.size() == 1 && m_contexts.contains(context)) { detachAndRemvoeAllObjects(); } thereby avoiding the re-adding and removal of the context.

done

>> Source/WebCore/html/canvas/WebGLContextGroup.cpp:89
>> +    removeObject(object);
> 
> Why is this call necessary? The are two other similar calls in WebGLRenderingContext.cpp, addSharedObject() and addContextObject().

done

>> Source/WebCore/html/canvas/WebGLContextObject.h:37
>> +class WebGLContextObject : public WebGLObject {
> 
> This class definitely needs a comment indicating that it represents WebGL resources that are per-context, rather than shared among contexts.

done

>> Source/WebCore/html/canvas/WebGLContextObject.h:67
>> +#endif // WebGLObject_h
> 
> Wrong #include guard.

done

>> Source/WebCore/html/canvas/WebGLFramebuffer.h:46
>> +    void setAttachmentForBoundFramebuffer(GraphicsContext3D*, GC3Denum attachment, GC3Denum texTarget, WebGLTexture*, GC3Dint level);
> 
> Since WebGLFramebuffer is a per-context object, it's unnecessary to pass in the GraphicsContext3D* here, as well as in removeAttachmentFromBoundFramebuffer and onAccess. I think the changes to those methods, and the calling code in WebGLRenderingContext, should be reverted in order to simplify the patch.

done

>> Source/WebCore/html/canvas/WebGLFramebuffer.h:-87
>> -    bool isBound() const;
> 
> Why remove this? It seems that the associated assertions would still be correct, since WebGLFramebuffer is a per-context object.

done

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4186
>> +    removeSharedObject(object);
> 
> Per comment above, I think this line should be removed.

done

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4198
>> +    removeContextObject(object);
> 
> Per comment above, I think this line should be removed.

done
Comment 11 Gregg Tavares 2012-01-12 13:31:07 PST 
Created attachment 122300 [details]
Patch
Comment 12 Kenneth Russell 2012-01-12 19:18:22 PST 
Comment on attachment 122300 [details]
Patch

Looks great. r=me
Comment 13 WebKit Review Bot 2012-01-13 11:02:32 PST 
Comment on attachment 122300 [details]
Patch

Clearing flags on attachment: 122300

Committed r104954: <http://trac.webkit.org/changeset/104954>
Comment 14 WebKit Review Bot 2012-01-13 11:02:38 PST 
All reviewed patches have been landed.  Closing bug.
Comment 15 Kenneth Russell 2012-01-13 11:19:41 PST 
Committed r104959: <http://trac.webkit.org/changeset/104959>
Comment 16 Kenneth Russell 2012-01-13 11:20:22 PST 
r104959 fixed an unused argument warning that wasn't seen when building locally on Mac.
Comment 17 Kenneth Russell 2012-01-13 11:41:37 PST 
Committed r104963: <http://trac.webkit.org/changeset/104963>
Comment 18 Kenneth Russell 2012-01-13 11:42:20 PST 
r104963 added the needed project.pbxproj changes that were somehow lost in the creation of the last patch.