Closed Bug 629675 Opened 13 years ago Closed 13 years ago

Get 3d context in xpcom for OpenCL interaction

Categories

(Core :: Graphics: CanvasWebGL, enhancement)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 664147

People

(Reporter: fcellier, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

modifications to access to the webGLcontext, buffer and texture ids
3.41 KB, patch
bjacob
: review-
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b11pre) Gecko/20110126 Firefox/4.0b11pre
Build Identifier: 4.0b11pre(2011-01-06)

I try to create an openCL binding for javascript in xpcom but part of the binding can't be done because i can't have OpenGL context from canvas (and WebGL context).

( content/canvas/src/WebGLContext.h:392:nsRefPtr<gl::GLContext> gl; nothing like this is exposed for xpcom component but id for buffer is (in nsIWebGLBuffer::getName))

Could we have an idl in order to have access to the context?

PS: sorry for my english :)

Reproducible: Always
we could add a [noscript] accessor for the GLContext*, but it wouldn't be usable from JS.  No plans to add IDL for GLContext itself though; you'd have to make calls on it from native code.
I already use c++ code so exporting WebGLContext.h and having an accessor is enough for me (i think :). Thanks
I don't know if it's a good solution but I use an abstract class (c++ interface) and a static cast to get the GLContext (so modifications are small).
After that i have to use GLXLibrary.h (or WGL) in order to configure openCL context with webGL interaction:

"""

cl_context_properties properties[] = { CL_GL_CONTEXT_KHR,
(cl_context_properties)glXGetCurrentContext(), CL_GLX_DISPLAY_KHR, (cl_context_properties)glXGetCurrentDisplay(),
CL_CONTEXT_PLATFORM, (cl_context_properties)platform,0};

cl_device_id devices[32]; size_t size;
clGetGLContextInfoKHR(properties, CL_DEVICES_FOR_GL_CONTEXT_KHR,
32 * sizeof(cl_device_id), devices, &size);

// Create a context using the supported devices
 int count = size / sizeof(cl_device_id);
 cl_context context = clCreateContext(properties, devices, count, NULL, 0, 0);
"""
mm, a better way is to just add a [notxpcom] function in the main rendering context IDL that just returns the GLContext*.  No need for the additional class/inheritance bit.
Ok, the new solution (I rebind gLName to xpcom name), but I don't like to have c++ import for GLContext in nsIDOMWebGLRenderingContext.idl

Dummy's questions : 
- I use  ***getCurrentDisplay*** which are not in XUL GLLibraries (gfx/thebes/GLContextProviderXXX), did I need to create a new patch? (I think it's no because I'm the only one which need it and I can use inheritence and dynamic linking to libxul.so :)

 - Is it ok if my xpcom component need a link with libXul? (for technical reason only, my code/poc will be on bitbucket with MPL/GPL/LGPL as soon as i have something good ...)
Attachment #508613 - Attachment is obsolete: true
Comment on attachment 509666 [details] [diff] [review]
modifications to access to the webGLcontext, buffer and texture ids

Putting 'review?' flag on vladimir ensures that he can't forget about it ;-)
Attachment #509666 - Flags: review?(vladimir)
Comment on attachment 509666 [details] [diff] [review]
modifications to access to the webGLcontext, buffer and texture ids

>+/* [noscript] DOM accessor for GLContext */
>+nsresult
>+WebGLContext::GetGLContext(gl::GLContext** context)
>+{
>+    *context=gl.get();

spaces around the =

>-NAME_NOT_SUPPORTED(WebGLTexture)
>-NAME_NOT_SUPPORTED(WebGLBuffer)
>+//NAME_NOT_SUPPORTED(WebGLTexture)
>+//NAME_NOT_SUPPORTED(WebGLBuffer)

Not entirely comfortable with this, though I understand the reasoning.  Is the getter marked noscript?  If you're going to do this, should probably change all this to NAME_NOT_FOR_CONTENT() and add in checks to the boilerplate to return an error if it's called from script without xpcom privileges.

>+//setter name for opencl interaction
>+#define NAME_SET_NOT_SUPPORTED(base) \
>+NS_IMETHODIMP base::GetName(WebGLuint *aName) \
>+{ *aName=GLName();return NS_OK; } \
>+NS_IMETHODIMP base::SetName(WebGLuint aName) \
>+{ return NS_ERROR_NOT_IMPLEMENTED; }

Whoa, definitely not comfortable with this.  Sorry, you can't just arbitrarily set this.

> NS_IMETHODIMP
>diff -r 019e47cc11e2 dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl
>--- a/dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl	Thu Feb 03 16:47:23 2011 -0800
>+++ b/dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl	Fri Feb 04 03:19:08 2011 +0100
>@@ -61,20 +61,26 @@ typedef float          WebGLclampf;
> 
> namespace js {
> struct ArrayBuffer;
> struct TypedArray;
> }
> 
> /* Avoid conflict with WinAPI */
> #undef NO_ERROR
>+
>+#include "GLContext.h"

just declare 'class GLContext;' inside a namespace mozilla { namespace gl { .. } } block.
Attachment #509666 - Flags: review?(vladimir) → review-
Are you not comfortable with giving GL ids to xpcom components (maybe i should use [noxpcom])?

I don't understand why we have so many "[noscript] attribute WebGLuint name;" in nsIDOMWebGLRenderingContext.idl if it's not used :)
Attachment #509666 - Attachment is obsolete: true
Attachment #510298 - Flags: review?(vladimir)
Comment on attachment 510298 [details] [diff] [review]
modifications to access to the webGLcontext, buffer and texture ids

OK, here's my belated (sorry!) review :-) It's almost all good as far as I can see.

>+/* [noscript] DOM accessor for GLContext */
>+nsresult
>+WebGLContext::GetGLContext(gl::GLContext** context)

Why nsresult and not NS_IMETHODIMP like the rest? Seems like using nsresult is going to give linking errors on Windows, right?

>+//[noscript] accessor :
>+//XXX should we replace GLName function with xpcom accessor?

That seems to be a great idea, right?
The XPCOM accessors are currently stubbed as "not implemented" in content/canvas/src/WebGLContext.cpp line 894:

#define NAME_NOT_SUPPORTED(base) \
NS_IMETHODIMP base::GetName(WebGLuint *aName) \
{ return NS_ERROR_NOT_IMPLEMENTED; } \
NS_IMETHODIMP base::SetName(WebGLuint aName) \
{ return NS_ERROR_NOT_IMPLEMENTED; }

NAME_NOT_SUPPORTED(WebGLTexture)
NAME_NOT_SUPPORTED(WebGLBuffer)
NAME_NOT_SUPPORTED(WebGLProgram)
NAME_NOT_SUPPORTED(WebGLShader)
NAME_NOT_SUPPORTED(WebGLFramebuffer)
NAME_NOT_SUPPORTED(WebGLRenderbuffer)

You are very welcome to make them actually implemented. Also, regarding the concern expressed above by Vlad, I am not worried about that: once we expose the GL context itself through XPCOM, for security purposes it's already as bad as exposing all the object names. Indeed, given the GL context, one could just iterate over integers from 0 to 1000 and call IsTexture(), IsProgram(), etc. So either we expose nothing or we expose everything.

>+#define XPCOM_GL_NAME_ACCESSOR(base) \
>+NS_IMETHODIMP base::GetGLName(WebGLuint *name) \
>+{\
>+    *name=mName;\
>+    return NS_OK;\
>+}\
>+
>+XPCOM_GL_NAME_ACCESSOR(WebGLBuffer)
>+XPCOM_GL_NAME_ACCESSOR(WebGLTexture)
>+XPCOM_GL_NAME_ACCESSOR(WebGLRenderbuffer)

FWIW, I wouldn't use a macro for that. I know that we use lots of macros, but that's not a good example to follow :-)
Attachment #510298 - Flags: review?(vladimir) → review-
Vlad: I don't understand your comment there:

> >+//setter name for opencl interaction
> >+#define NAME_SET_NOT_SUPPORTED(base) \
> >+NS_IMETHODIMP base::GetName(WebGLuint *aName) \
> >+{ *aName=GLName();return NS_OK; } \
> >+NS_IMETHODIMP base::SetName(WebGLuint aName) \
> >+{ return NS_ERROR_NOT_IMPLEMENTED; }
> 
> Whoa, definitely not comfortable with this.  Sorry, you can't just arbitrarily
> set this.

What did you mean here? His code didn't allow to set this, since the setter was just returning NS_ERROR_NOT_IMPLEMENTED. So this seems just fine to me.
(In reply to comment #5)
> Created attachment 509666 [details] [diff] [review]
> modifications to access to the webGLcontext, buffer and texture ids
> 
> Ok, the new solution (I rebind gLName to xpcom name), but I don't like to have
> c++ import for GLContext in nsIDOMWebGLRenderingContext.idl

What does "c++ import" mean here?

> 
> Dummy's questions : 
> - I use  ***getCurrentDisplay*** which are not in XUL GLLibraries
> (gfx/thebes/GLContextProviderXXX), did I need to create a new patch? (I think
> it's no because I'm the only one which need it and I can use inheritence and
> dynamic linking to libxul.so :)

Not sure why you would need such low-level non-portable system functions as that... but there is nothing wrong with loading this symbol yourself and I don't see why you would need support from libXUL for that.

> 
>  - Is it ok if my xpcom component need a link with libXul? (for technical
> reason only, my code/poc will be on bitbucket with MPL/GPL/LGPL as soon as i
> have something good ...)

Why would you need linking to libXUL? XPCOM should precisely make that unneeded!
> 
> What does "c++ import" mean here?
Sorry, I mean include.

> 
> Not sure why you would need such low-level non-portable system functions as
> that... but there is nothing wrong with loading this symbol yourself and I
> don't see why you would need support from libXUL for that.
> 
If I want OpenCL's Context which interact with (OpenGL(ES) | DirextX10 or +),i need context and display (hdc, glxDisplay...) 
A better way is to add a getter to "NativeDisplay" in GLContext::NativeDataType.

> 
> Why would you need linking to libXUL? XPCOM should precisely make that
> unneeded!
Ok, i tried to change that, but problem is that I interact with typedArray (JS_FRIEND_API). Nothing to do with our problem here:)
Summary: Get 3d context in xpcom → Get 3d context in xpcom for OpenCL interaction
(In reply to comment #12)
> > 
> > What does "c++ import" mean here?
> Sorry, I mean include.

Oh! I didn't see that. We can't do that: I am actually very surprised that this worked!

> > 
> > Not sure why you would need such low-level non-portable system functions as
> > that... but there is nothing wrong with loading this symbol yourself and I
> > don't see why you would need support from libXUL for that.
> > 
> If I want OpenCL's Context which interact with (OpenGL(ES) | DirextX10 or +),i
> need context and display (hdc, glxDisplay...) 
> A better way is to add a getter to "NativeDisplay" in
> GLContext::NativeDataType.

OK. This raises the same kind of concerns as Vlad expressed above about exposing internal OpenGL handles, so let's resolve that first.

> 
> > 
> > Why would you need linking to libXUL? XPCOM should precisely make that
> > unneeded!
> Ok, i tried to change that, but problem is that I interact with typedArray
> (JS_FRIEND_API). Nothing to do with our problem here:)

OK.

All that together strongly suggests to me that instead of an external XPCOM component, all that would be better developed as a fork of mozilla-central.
> All that together strongly suggests to me that instead of an external XPCOM
> component, all that would be better developed as a fork of mozilla-central.

Done, fork : https://bitbucket.org/tallion/gecko-webcl/, i'll sync with central as often as i can and my work will be add from https://bitbucket.org/tallion/webclworker/ to webcl branch (with modification due to the fact the goal is not an extension anymore).

(I create branch and not mq because people can use this fork for developing their own patches on bitbucket :)
(In reply to comment #14)
> > All that together strongly suggests to me that instead of an external XPCOM
> > component, all that would be better developed as a fork of mozilla-central.
> 
> Done, fork : https://bitbucket.org/tallion/gecko-webcl/

Excellent, thanks. We could also have done that at hg.mozilla.org/projects.

Anyone who already has a mozilla-central clone and wanting to quickly clone that:

cp -r mozilla-central gecko-webcl
cd gecko-webcl
edit .hg/hgrc to let it point to the bitbucket repo
hg pull -u
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: