1056947 - WEBGL_draw_buffers extension incorrectly exposed in Firefox on some GLES3 devices

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Olli Etuaho]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140715215003

Steps to reproduce:

Run https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-draw-buffers.html on a device with Tegra K1.


Actual results:

Test failed, first failure:
FAIL fragment shader containing the #extension directive should compile


Expected results:

Extension should be reported as not supported - or alternatively shaders should be translated under the hood to GLSL 3.0. GLES 3.0 section 4.2.1 specifies that GLSL 1.0 shaders only write to the first draw buffer, so on devices supporting GLES 3.0 but not EXT_draw_buffers GLSL 1.0 shaders can not write to multiple draw buffers.

Comment 1

[Aaron Train [:aaronmt]]

What device or board are you using with a K1? The Jetson TK1?

Comment 2

[Olli Etuaho]

I was using an internal development board similar to that one. Results should be the same on Shield Tablet, which recently came out.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

It looks like we already have code to only enable that extension on GLES3 (or desktop GL 2.0), so I'm not sure what's going wrong here. AFAICT, we always have a GLES2 context on EGL.

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> It looks like we already have code to only enable that extension on GLES3
> (or desktop GL 2.0), so I'm not sure what's going wrong here. AFAICT, we
> always have a GLES2 context on EGL.

Sometimes we get a GLES3 context, because EGL is allowed to give us GLES3 when we as for GLES2. This happens on at least my Nexus 10. (IIRC)

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> > It looks like we already have code to only enable that extension on GLES3
> > (or desktop GL 2.0), so I'm not sure what's going wrong here. AFAICT, we
> > always have a GLES2 context on EGL.
> 
> Sometimes we get a GLES3 context, because EGL is allowed to give us GLES3
> when we as for GLES2. This happens on at least my Nexus 10. (IIRC)

Ok, but unless I'm missing something, the GLContextFeatures stuff uses what we give to GLContext::SetProfileVersion -- which is always 200 for EGL. So what's going on?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Oh I'm misunderstanding how this works. If we have GLES3 *OR* EXT_draw_buffers then the feature is enabled. That seems to be wrong according to Olli.

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> (In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > (In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> > > It looks like we already have code to only enable that extension on GLES3
> > > (or desktop GL 2.0), so I'm not sure what's going wrong here. AFAICT, we
> > > always have a GLES2 context on EGL.
> > 
> > Sometimes we get a GLES3 context, because EGL is allowed to give us GLES3
> > when we as for GLES2. This happens on at least my Nexus 10. (IIRC)
> 
> Ok, but unless I'm missing something, the GLContextFeatures stuff uses what
> we give to GLContext::SetProfileVersion -- which is always 200 for EGL. So
> what's going on?

Ah, that's incorrectly hardcoded. (new bug 1060072)

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > > (In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> > > > It looks like we already have code to only enable that extension on GLES3
> > > > (or desktop GL 2.0), so I'm not sure what's going wrong here. AFAICT, we
> > > > always have a GLES2 context on EGL.
> > > 
> > > Sometimes we get a GLES3 context, because EGL is allowed to give us GLES3
> > > when we as for GLES2. This happens on at least my Nexus 10. (IIRC)
> > 
> > Ok, but unless I'm missing something, the GLContextFeatures stuff uses what
> > we give to GLContext::SetProfileVersion -- which is always 200 for EGL. So
> > what's going on?
> 
> Ah, that's incorrectly hardcoded. (new bug 1060072)

Actually it appears that bug is already "fixed"[0]. So if we get a GLES3 context, we mark the draw_buffers feature as supported. As far as WebGL is concerned, though, it really isn't, because people expect the #extension to work (according to spec). We either need to mark it unsupported or rewrite the shader by stripping the #extension line.

http://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContext.cpp?from=InitWithPrefix&case=true#495

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

BTW this bug is not exclusive to the TK1. I reproduced it on the Nexus 10 and LG G2. I'd guess any KitKat device has a decent chance of having the bug.

Comment 10

[Olli Etuaho]

Just stripping the #extension line when running on GLES3 is not enough. Multiple render targets are not supported on GLES3 with GLSL ES 1.00 shaders, see GLSL ES 3.00 specification section 1.5, compatibility for a clearer mention of this than GLES3.0 section 4.2.1.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't enable GLFeature::draw_buffers (and therefore WEBGL_draw_buffers) on GLES3

This patch makes it so the extension is required on GLES in order to enable GLFeature::draw_buffers

Given the differences between desktop GL/EXT_draw_buffers and GLES3 I think this makes sense. We may want to add a different feature for the draw_buffers supported by GLES3.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Hmm, so WEBGL_draw_buffers is 'natively' supported in WebGL2. The spec there seems to indicate that you need the #extension directive no matter what, and does not address the lack of multiple render buffer support, etc. I think I'll just let Jeff handle this :)

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8481333 [details] [diff] [review]
Don't enable GLFeature::draw_buffers (and therefore WEBGL_draw_buffers) on GLES3

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

Can you leave a comment with a note about this bug? Otherwise we might forget about this later.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3389d2ffb11

Comment 16

[Olli Etuaho]

Thanks for the fix. Note that we discovered there should be an alternative way to implement this WEBGL extension on NVIDIA Tegra platforms which support GLES3.0 and NV_draw_buffers, by using core GLES3.0 drawBuffers function and NV_draw_buffers #extension directive.