Closed Bug 1056947 Opened 6 years ago Closed 6 years ago

WEBGL_draw_buffers extension incorrectly exposed in Firefox on some GLES3 devices

Categories

(Core :: Canvas: WebGL, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
fennec + ---

People

(Reporter: oetuaho, Assigned: jgilbert)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
Component: General → Graphics, Panning and Zooming
OS: Linux → Android
Hardware: x86_64 → ARM
Component: Graphics, Panning and Zooming → Canvas: WebGL
Product: Firefox for Android → Core
Version: Firefox 31 → unspecified
What device or board are you using with a K1? The Jetson TK1?
I was using an internal development board similar to that one. Results should be the same on Shield Tablet, which recently came out.
tracking-fennec: --- → ?
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.
tracking-fennec: ? → +
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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)
(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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?
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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #5)
> (In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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)
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #5)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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
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.
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.
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.
Attachment #8481333 - Flags: review?(jgilbert)
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 :)
Assignee: nobody → 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.
Attachment #8481333 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/a3389d2ffb11
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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.
See Also: → 1300932
Depends on: 1513207
You need to log in before you can comment on or make changes to this bug.