Bug 75906 - Add WebGLContextGroup as step toward sharing WebGL resources
Summary: Add WebGLContextGroup as step toward sharing WebGL resources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gregg Tavares
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-09 16:24 PST by Gregg Tavares
Modified: 2012-01-13 11:42 PST (History)
5 users (show)

See Also:


Attachments
Patch (79.63 KB, patch)
2012-01-09 17:21 PST, Gregg Tavares
no flags Details | Formatted Diff | Diff
Patch (79.79 KB, patch)
2012-01-10 12:48 PST, Gregg Tavares
no flags Details | Formatted Diff | Diff
Patch (89.43 KB, patch)
2012-01-10 17:01 PST, Gregg Tavares
no flags Details | Formatted Diff | Diff
Patch (90.89 KB, patch)
2012-01-10 18:20 PST, Gregg Tavares
no flags Details | Formatted Diff | Diff
Patch (76.74 KB, patch)
2012-01-12 13:28 PST, Gregg Tavares
no flags Details | Formatted Diff | Diff
Patch (76.74 KB, patch)
2012-01-12 13:31 PST, Gregg Tavares
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.