565404 - Uniform methods need to use a WebGLUniformLocation object, not an integer

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

As per the spec, we need to wrap uniform locations in an object and not pass a bare integer around.  We do this for textures, shaders, and other objects; it just wasn't done for uniforms.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Rough patch

Here's a rough patch. Not asking for formal review as it's not ready yet, just asking for help:

***************************************************

1) This patch makes firefox crash on startup here, very early, even without loading any webgl page. The backtrace, below, seems completely unrelated. My only guess would be that I made a mistake in the idl ?? Please help.

#0  0x00007f929c98477d in nanosleep () from /lib/libc.so.6
#1  0x00007f929c98460f in sleep () from /lib/libc.so.6
#2  0x00007f92a173e1f0 in ah_crap_handler (signum=11) at ../../../../mozilla-central/toolkit/xre/nsSigHandlers.cpp:132
#3  0x00007f92a1742b16 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7fff6273e530, context=0x7fff6273e400)
    at nsProfileLock.cpp:221
#4  <signal handler called>
#5  0x00007f92a1fef347 in NotifyDocumentTree (aDocument=0x0, aData=0x0)
    at ../../../../mozilla-central/dom/base/nsGlobalWindow.cpp:6960
#6  0x00007f92a1fef3cd in nsGlobalWindow::SetActive (this=0x7f928f1a5400, aActive=1)
    at ../../../../mozilla-central/dom/base/nsGlobalWindow.cpp:6969
#7  0x00007f92a1fef2ee in nsGlobalWindow::ActivateOrDeactivate (this=0x7f928f1a5400, aActivate=1)
    at ../../../../mozilla-central/dom/base/nsGlobalWindow.cpp:6953
#8  0x00007f92a1fc827a in nsFocusManager::WindowRaised (this=0x7f928f6fdb70, aWindow=0x7f928f1a5400)
    at ../../../../mozilla-central/dom/base/nsFocusManager.cpp:659
#9  0x00007f92a25fb1e1 in nsWebShellWindow::HandleEvent (aEvent=0x7fff6273ed00)
    at ../../../../../mozilla-central/xpfe/appshell/src/nsWebShellWindow.cpp:438
#10 0x00007f92a2874fa6 in nsWindow::DispatchEvent (this=0x7f928f264890, aEvent=0x7fff6273ed00, 
    aStatus=@0x7fff6273ed4c) at ../../../../../mozilla-central/widget/src/gtk2/nsWindow.cpp:586
#11 0x00007f92a2874e84 in nsWindow::DispatchActivateEvent (this=0x7f928f264890)
    at ../../../../../mozilla-central/widget/src/gtk2/nsWindow.cpp:557
#12 0x00007f92a287b531 in nsWindow::OnContainerFocusInEvent (this=0x7f928f264890, aWidget=0x7f928fd18e80, 
    aEvent=0x7f928fd05e40) at ../../../../../mozilla-central/widget/src/gtk2/nsWindow.cpp:3121
#13 0x00007f92a2882b46 in focus_in_event_cb (widget=0x7f928fd18e80, event=0x7f928fd05e40)
    at ../../../../../mozilla-central/widget/src/gtk2/nsWindow.cpp:5868
#14 0x00007f929faea288 in _gtk_marshal_BOOLEAN__BOXED () from /usr/lib/libgtk-x11-2.0.so.0
#15 0x00007f929e01751e in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
#16 0x00007f929e025fa6 in signal_emit_unlocked_R () from /usr/lib/libgobject-2.0.so.0
#17 0x00007f929e02f195 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#18 0x00007f929e02f5c3 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
#19 0x00007f929fc0035f in gtk_widget_event_internal () from /usr/lib/libgtk-x11-2.0.so.0
#20 0x00007f929fc0ce13 in do_focus_change () from /usr/lib/libgtk-x11-2.0.so.0
#21 0x00007f929fc0f068 in _gtk_window_set_is_active () from /usr/lib/libgtk-x11-2.0.so.0
#22 0x00007f929fc0f117 in gtk_window_focus_in_event () from /usr/lib/libgtk-x11-2.0.so.0
#23 0x00007f929faea288 in _gtk_marshal_BOOLEAN__BOXED () from /usr/lib/libgtk-x11-2.0.so.0
#24 0x00007f929e01751e in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
#25 0x00007f929e025dac in signal_emit_unlocked_R () from /usr/lib/libgobject-2.0.so.0
#26 0x00007f929e02f195 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#27 0x00007f929e02f5c3 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
---Type <return> to continue, or q <return> to quit---
#28 0x00007f929fc0035f in gtk_widget_event_internal () from /usr/lib/libgtk-x11-2.0.so.0
#29 0x00007f929fae3a65 in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
#30 0x00007f929eb9ae3c in gdk_event_dispatch () from /usr/lib/libgdk-x11-2.0.so.0
#31 0x00007f929d75ab33 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#32 0x00007f929d75b310 in g_main_context_iterate () from /usr/lib/libglib-2.0.so.0
#33 0x00007f929d75b5ad in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#34 0x00007f92a2887f7c in nsAppShell::ProcessNextNativeEvent (this=0x7f928fd73080, mayWait=1)
    at ../../../../../mozilla-central/widget/src/gtk2/nsAppShell.cpp:144
#35 0x00007f92a28acd45 in nsBaseAppShell::DoProcessNextNativeEvent (this=0x7f928fd73080, mayWait=1)
    at ../../../../../mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:156
#36 0x00007f92a28ad169 in nsBaseAppShell::OnProcessNextEvent (this=0x7f928fd73080, thr=0x7f92981380d0, mayWait=1, 
    recursionDepth=0) at ../../../../../mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:312
#37 0x00007f92a2b271cb in nsThread::ProcessNextEvent (this=0x7f92981380d0, mayWait=1, result=0x7fff6273fa2c)
    at ../../../../mozilla-central/xpcom/threads/nsThread.cpp:517
#38 0x00007f92a2abbad0 in NS_ProcessNextEvent_P (thread=0x7f92981380d0, mayWait=1) at nsThreadUtils.cpp:250
#39 0x00007f92a29f716b in mozilla::ipc::MessagePump::Run (this=0x7f92981dc340, aDelegate=0x7f9298155840)
    at ../../../../mozilla-central/ipc/glue/MessagePump.cpp:142
#40 0x00007f92a2b9bc8b in MessageLoop::RunInternal (this=0x7f9298155840)
    at ../../../../mozilla-central/ipc/chromium/src/base/message_loop.cc:216
#41 0x00007f92a2b9bc10 in MessageLoop::RunHandler (this=0x7f9298155840)
    at ../../../../mozilla-central/ipc/chromium/src/base/message_loop.cc:199
#42 0x00007f92a2b9bba1 in MessageLoop::Run (this=0x7f9298155840)
    at ../../../../mozilla-central/ipc/chromium/src/base/message_loop.cc:173
#43 0x00007f92a28acdd1 in nsBaseAppShell::Run (this=0x7f928fd73080)
    at ../../../../../mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:175
#44 0x00007f92a261f36d in nsAppStartup::Run (this=0x7f928f6d3420)
    at ../../../../../../mozilla-central/toolkit/components/startup/src/nsAppStartup.cpp:192
#45 0x00007f92a172fd57 in XRE_main (argc=4, argv=0x7fff62740648, aAppData=0x7f92981250f0)
    at ../../../../mozilla-central/toolkit/xre/nsAppRunner.cpp:3612
#46 0x000000000040214c in main (argc=4, argv=0x7fff62740648)
    at ../../../../mozilla-central/browser/app/nsBrowserApp.cpp:158



***************************************************

2) This patch only fixes the non-array methods, glUniform[1-4][fi]
Now must dp the same for the 'v' methods. Is it correct that all I have to change is CustomQS_WebGL.h, no need to do any more changes in nsICanvasRenderingContextWebGL.idl? It is correct that I need to change these 4 functions,

    helper_nsICanvasRenderingContextWebGL_Uniform_x_iv
    helper_nsICanvasRenderingContextWebGL_Uniform_x_fv
    helper_nsICanvasRenderingContextWebGL_Uniform_x_iv_tn
    helper_nsICanvasRenderingContextWebGL_Uniform_x_fv_tn

?

In the non-tn functions, this looks easy, I just need to compute 'location' from argv[0].

In the tn functions, I'm not so sure. What should be the type of the 'location' parameter passed to these functions?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding WebGLUniformLocation class

This time it's ready for review!

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding WebGLUniformLocation class, updated

Updated to apply cleanly against current mozilla-central

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> diff --git a/content/canvas/src/WebGLContext.h b/content/canvas/src/WebGLContext.h
> --- a/content/canvas/src/WebGLContext.h
> +++ b/content/canvas/src/WebGLContext.h
> +#define WEBGLUNIFORMLOCATION_PRIVATE_IID \
> +    {0x01a8a614, 0xb109, 0x42f1, {0xb4, 0x40, 0x8d, 0x8b, 0x87, 0x0b, 0x43, 0xa7}}
> +class WebGLUniformLocation :
> +    public nsIWebGLUniformLocation,
> +    public WebGLZeroingObject
> +{
> +public:
> +    NS_DECLARE_STATIC_IID_ACCESSOR(WEBGLUNIFORMLOCATION_PRIVATE_IID)
> +
> +    WebGLUniformLocation(WebGLProgram *program, GLint location) :
> +        mProgram(program), mLocation(location), mDeleted(PR_FALSE) { }
> +
> +    void Delete() {
> +        if (mDeleted)
> +            return;
> +        ZeroOwners();
> +        mDeleted = PR_TRUE;
> +    }
> +    PRBool Deleted() { return mDeleted; }
 
^ Uniform locations can't be deleted -- no need for mDeleted or these functions.

> +    WebGLProgram *program() const { return mProgram; }
> +    GLint location() const { return mLocation; } 

^ Capitalized method names!

> diff --git a/content/canvas/src/WebGLContextGL.cpp b/content/canvas/src/WebGLContextGL.cpp
> --- a/content/canvas/src/WebGLContextGL.cpp
> +++ b/content/canvas/src/WebGLContextGL.cpp
> @@ -60,47 +60,47 @@ using namespace mozilla;
>  
>  static PRBool BaseTypeAndSizeFromUniformType(WebGLenum uType, WebGLenum *baseType, WebGLint *unitSize);
>  
>  /* Helper macros for when we're just wrapping a gl method, so that
>   * we can avoid having to type this 500 times.  Note that these MUST
>   * NOT BE USED if we need to check any of the parameters.
>   */
>  
> -#define GL_SAME_METHOD_0(glname, name)                          \
> +#define SAME_METHOD_0(glname, name)                          \
 
^ This renaming is fine, but don't do it in the same patch as this -- this is just mechanical search-and-replace noise and it makes reviewing the actual work of this patch harder, especially when the renaming is unrelated.
 
> @@ -115,17 +115,17 @@ GetConcreteObject(BaseInterfaceType *aIn
>                    ConcreteObjectType **aConcreteObject,
>                    PRBool *isNull = 0,
>                    PRBool *isDeleted = 0)
>  {
>      if (!aInterface) {
>          if (NS_LIKELY(isNull)) {
>              // non-null isNull means that the caller will accept a null arg
>              *isNull = PR_TRUE;
> -            if(isDeleted) *isDeleted = PR_FALSE;
> +            if (isDeleted) *isDeleted = PR_FALSE;
 
^ same thing -- syntax cleanups are fine, but not in the same patch as real work (unless they're simple drive-by cleanups in an area that you happen to be in)

>  /* XXX fix */
>  /* any getUniform(in WebGLProgram program, in WebGLUniformLocation location) raises(DOMException); */
>  NS_IMETHODIMP
> -WebGLContext::GetUniform(nsIWebGLProgram *pobj, WebGLint location)
> +WebGLContext::GetUniform(nsIWebGLProgram *pobj, nsIWebGLUniformLocation *ploc)
>  {
>      WebGLuint progname;
> -    if (!GetGLName<WebGLProgram>(pobj, &progname))
> +    WebGLProgram *prog;
> +    if (!GetConcreteObjectAndGLName(pobj, &prog, &progname))
>          return ErrorInvalidOperation("GetUniform: invalid program");
>  
> +    WebGLUniformLocation *location;
> +    if (!GetConcreteObject(ploc, &location))
> +        return NS_ERROR_DOM_SYNTAX_ERR;

^ Wrong error, though the spec doesn't say what it should be.  I would say ErrorInvalidValue here.

> +    if (location->program() != prog)
> +        return NS_ERROR_FAILURE;

^ Same thing, ErrorInvalidValue again.

>      NativeJSContext js;
>      if (NS_FAILED(js.error))
>          return js.error;
>  
>      MakeContextCurrent();
>  
> -    GLint uArraySize = 0;
> -    GLenum uType = 0;
> -
> -    gl->fGetActiveUniform(progname, location, 0, NULL, &uArraySize, &uType, NULL);
> -    if (uArraySize == 0)
> +    WebGLint activeUniforms = 0;
> +    gl->fGetProgramiv(progname, LOCAL_GL_ACTIVE_UNIFORMS, &activeUniforms);
> +
> +    // we now need the type info to switch between fGetUniformfv and fGetUniformiv
> +    // the only way to get that is to iterate through all active uniforms by index until
> +    // one matches the given uniform location.
> +    WebGLenum uniformType = 0;
> +    WebGLint index;
> +    for (index = 0; index < activeUniforms; ++index) {
> +        GLsizei dummyLength;
> +        GLchar uniformName[256];
> +        WebGLint dummySize;
> +        gl->fGetActiveUniform(progname, index, 255, &dummyLength, &dummySize, &uniformType, uniformName);
> +        if (gl->fGetUniformLocation(progname, uniformName) == location->location())
> +            break;
> +    }
> +
> +    if (index == activeUniforms)
>          return NS_ERROR_FAILURE; // XXX GL error? shouldn't happen.

^ We should really do this just once per program, and cache it on the WebGLProgram.  (Lookup all the uniforms and cache the index -> location).  Though this is a getter anyway, so this isn't that big of a deal.  You also want to query GL_ACTIVE_UNIFORM_MAX_LENGTH to get the maximum string length, and use a nsCAutoString to simplify the length management -- take a look at how GetProgramInfoLog does it.

>  NS_IMETHODIMP
> -WebGLContext::GetUniformLocation(nsIWebGLProgram *pobj, const nsAString& name, WebGLint *retval)
> +WebGLContext::GetUniformLocation(nsIWebGLProgram *pobj, const nsAString& name, nsIWebGLUniformLocation **retval)
>  {
>      WebGLuint progname;
> -    if (!GetGLName<WebGLProgram>(pobj, &progname))
> +    WebGLProgram *prog;
> +    if (!GetConcreteObjectAndGLName(pobj, &prog, &progname))
>          return ErrorInvalidOperation("GetUniformLocation: invalid program");
>  
>      MakeContextCurrent();
> -    *retval = gl->fGetUniformLocation(progname, NS_LossyConvertUTF16toASCII(name).get());
> +
> +    WebGLint intlocation = gl->fGetUniformLocation(progname, NS_LossyConvertUTF16toASCII(name).get());
> +
> +    nsCOMPtr<WebGLUniformLocation> uloc = new WebGLUniformLocation(prog, intlocation);
> +    if (uloc) {
> +        *retval = uloc.forget().get();
> +    }

^ No need for the if -- all mallocs/news are infallible unless explicitly marked as fallible in the gecko codebase.

> -#define GL_SIMPLE_ARRAY_METHOD(name, cnt, arrayType, ptrType)           \
> +#define OBTAIN_UNIFORM_LOCATION \
> +    WebGLUniformLocation *location_object; \
> +    if (!GetConcreteObject(ploc, &location_object)) \
> +        return NS_ERROR_DOM_SYNTAX_ERR; \

^ Return a better error; use invalid value as per above.

This also needs to check that the location matches the current program -- it's in mCurrentProgram.

Also formatting -- align the \'s at the end.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding WebGLUniformLocation class, again updated

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

I think that the new patch addresses all your comments, except:


> ^ Uniform locations can't be deleted -- no need for mDeleted or
> these functions.

ok, I removed most of that, but had to leave a dummy Deleted() function to be able to use my GetConcreteObject() helper. I hope it's ok. otherwise we can do stuff by hand.

> ^ We should really do this just once per program, and cache it on the
> WebGLProgram.  (Lookup all the uniforms and cache the index -> location). 
> Though this is a getter anyway, so this isn't that big of a deal.

I will file a separate bug for that.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

> I will file a separate bug for that.

Bug 569432

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding WebGLUniformLocation class, again updated

Update: undo some abusive GLtype --> WebGLtype changes.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Wait, don't push! There's a bug in the current version of that patch (introduced after your r+).

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding WebGLUniformLocation class, good to review

OK this time it's ready for review! The bug was that I was using LOCAL_GL_ACTIVE_ATTRIBUTE_MAX_LENGTH instead of LOCAL_GL_ACTIVE_UNIFORM_MAX_LENGTH.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 448626 [details] [diff] [review]
Patch adding WebGLUniformLocation class, good to review

Looks good, thanks!

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/6b2cd524bced