Bug 75906 – Add WebGLContextGroup as step toward sharing WebGL resources

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

[Gregg Tavares]

Add WebGLContextGroup as step toward sharing WebGL resources

[Gregg Tavares]

Created attachment 121765 [details]
Patch

[Gregg Tavares]

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.

[Kenneth Russell]

Looks like the patch needs to be rebaselined to TOT. Could you do that and resubmit?

[Gregg Tavares]

Created attachment 121894 [details]
Patch

[Early Warning System Bot]

Comment on attachment 121894 [details]
Patch

Attachment 121894 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11167562

[Gregg Tavares]

Created attachment 121944 [details]
Patch

[Gregg Tavares]

Created attachment 121956 [details]
Patch

[Kenneth Russell]

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.

[Gregg Tavares]

Created attachment 122297 [details]
Patch

[Gregg Tavares]

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

[Gregg Tavares]

Created attachment 122300 [details]
Patch

[Kenneth Russell]

Comment on attachment 122300 [details]
Patch

Looks great. r=me

[WebKit Review Bot]

Comment on attachment 122300 [details]
Patch

Clearing flags on attachment: 122300

Committed r104954: <http://trac.webkit.org/changeset/104954>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

Committed r104959: <http://trac.webkit.org/changeset/104959>

[Kenneth Russell]

r104959 fixed an unused argument warning that wasn't seen when building locally on Mac.

[Kenneth Russell]

Committed r104963: <http://trac.webkit.org/changeset/104963>

[Kenneth Russell]

r104963 added the needed project.pbxproj changes that were somehow lost in the creation of the last patch.