750527 - NV GL driver messes up when relinking the currently-bound program

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

This is the conformance failure we're seeing on:
uniforms/uniform-location.html

This is fixed simply by rebinding the program after relink if it is the current program.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch

Comment 2

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=dedf969dcf53

Comment 3

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=d8dc91c6c0ea

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 619714 [details] [diff] [review]
Patch

Review of attachment 619714 [details] [diff] [review]:
-----------------------------------------------------------------

Limit this to nVidia HW and annotate this workaround with this bug number, similar to the existing:
// bug 744888
        if (WorkAroundDriverBugs() &&
            !data &&
            Vendor() == VendorNVIDIA)

Comment 5

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

Comment on attachment 619714 [details] [diff] [review]
Patch

Review of attachment 619714 [details] [diff] [review]:
-----------------------------------------------------------------

Waiting for updated patch then. OK in principle.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch

Fixed up with bug comment and limited to NV hardware.
I also added a function fGetUIntegerv, which replaces this common meme:
GLuint name = 0;
fGetIntegerv(LOCAL_GL_PARAM, (GLint*)&name);

With:
GLuint name = 0;
fGetUIntegerv(LOCAL_GL_PARAM, &name);

Comment 7

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=6b430c5babba

Comment 8

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

Comment on attachment 620134 [details] [diff] [review]
Patch

Review of attachment 620134 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextGL.cpp
@@ +3347,5 @@
> +            updateInfoSucceeded &&
> +            gl->Vendor() == gl::GLContext::VendorNVIDIA)
> +        {
> +            GLuint curProgram = 0;
> +            gl->fGetUIntegerv(LOCAL_GL_CURRENT_PROGRAM, &curProgram);

Wait! if all you need to know is whether this program is the current one, you don't need to query the GL for that. You can simply compare the pointers to WebGLProgram objects. Compare |program| to |mCurrentProgram|.

::: gfx/gl/GLContext.h
@@ +1890,5 @@
>      }
>  
> +    void fGetUIntegerv(GLenum pname, GLuint* param) {
> +        fGetIntegerv(pname, (GLint*)param);
> +    }

Since this function is not in the real GL API, it shouldn't have this 'f' prefix, which comes from these functions originally being the raw function pointers loaded from GL libs.

But see other comment, it shouldn't be needed.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 10

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec507e41777f

Comment 11

[Kelsey Gilbert [:jgilbert]]

Previous landing was a null push.

Actual push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60ac5e1aba15

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60ac5e1aba15

Comment 13

[Kenneth Russell]

FYI, this has been filed as NVIDIA Incident 110263 and is tracked on the WebGL WG's internal wiki.