ANGLE already includes the ability to translate EXT_shader_texture_lod to ARB_shader_texture_lod (e.g. changing EXT to ARB suffix) when using GLSL output. We don't enable WebGL EXT_shader_texture_lod unless the underlying implementation supports only EXT_shader_texture_lod though; we should fix this so that ARB_shader_texture_lod will also trigger support. This affects exposing this WebGL extension on OSX and Linux.

ARB_shader_texture_lod is contained in the core profile of OpenGL 3.0 and OpenGL ES 3.0. Support should also trigger if a core or compatibility profile is used that's higher than 3.0, regardless of the presence or absence of EXT/ARB_shader_texture_lod.

Comment on attachment 8536699 [details] [diff] [review] This is a start, I imagine there is more? I actually am not sure -- that *may* be all that's needed. Do a build on OSX and see if the test passes? :) Part of the problem here is that the extension tests pass if the extension is not available. Ideally we'd have a way to say "we expect this extension to be available" -- maybe we need a mozilla-specific test for that, to verify that we're actually testing what we expect to. (That's for a different bug, but it would just need to check that we have a set of extensions that we expect on our various test machines.)

(In reply to Florian Bösch from comment #2) > ARB_shader_texture_lod is contained in the core profile of OpenGL 3.0 and > OpenGL ES 3.0. Support should also trigger if a core or compatibility > profile is used that's higher than 3.0, regardless of the presence or > absence of EXT/ARB_shader_texture_lod. Yeah, we should probably handle this via a meta feature in GLContextFeatures (shader_texture_lod) that's provided by EXT_, ARB_, or GL >= 3.0. We don't currently create a core profile on OSX, but would like to at some point in the future. However, if we do provide it via core, I think ANGLE will need to be updated -- right now ANGLE's GLSL translator only ouputs the proper translation for the ARB extension. It will need to understand how to output the core symbols as well, if GL >= 3.0.

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #3) > Comment on attachment 8536699 [details] [diff] [review] > This is a start, I imagine there is more? > > I actually am not sure -- that *may* be all that's needed. Do a build on > OSX and see if the test passes? :) Which test?

This bug is preventing use of the extension on OSX (and Linux?). Ken Russell suggested that Chrome already supports ARB_shader_texture_lod, and exposes the extension on OSX. Can Firefox do the same? This is important for texture ops in loops/conditionals, since some shader optimizers unrolled/extracted texture ops that needed the derivatives.

Can somebody give me a (minimal?) test case that fails in Firefox and passes in Chrome because of this issue?

This is the khronos test case. Firefox reports that the extension isn't exposed on OSX 10.10.1. https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/ext-shader-texture-lod.html

Comment on attachment 8536699 [details] [diff] [review] This is a start, I imagine there is more? That may be enough. Ken Russell said the Angle shader translator works with ARB_ or EXT_shader_texture_lod. #extension and the call itself texture2DLodExt vs. textureLod() in core 3.2. I'm not sure if texture2DLodARB() in 2.1, since I only use core 3.2+. If you run that test and it passes, then that should be enough.

Comment on attachment 8536699 [details] [diff] [review] This is a start, I imagine there is more? Review of attachment 8536699 [details] [diff] [review]: ----------------------------------------------------------------- Yep. It should need something in the GLFeature configs, but we should double-check with the specs.

(In reply to Alecazam from comment #8) > This is the khronos test case. Firefox reports that the extension isn't > exposed on OSX 10.10.1. > > https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/ext- > shader-texture-lod.html Passes and supports it with the patch.

Comment on attachment 8561303 [details] [diff] [review] ARB_shader_texture_lod support to implement WebGL EXT_shader_texture_lod. r=jgilbert Review of attachment 8561303 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContextExtensions.cpp @@ +115,5 @@ > case WebGLExtensionID::EXT_frag_depth: > return WebGLExtensionFragDepth::IsSupported(this); > case WebGLExtensionID::EXT_shader_texture_lod: > + return gl->IsExtensionSupported(gl::GLContext::EXT_shader_texture_lod) || > + gl->IsExtensionSupported(gl::GLContext::ARB_shader_texture_lod); This should be: return gl->IsSupported(gl::GLFeature::shader_texture_lod); ::: gfx/gl/GLContext.h @@ +122,1 @@ > sampler_objects, Please keep these sorted: ... sampler_objects, shader_texture_lod, ... (Yeah, sR... comes before sa... according to M-x sort-lines. Unix FTW) ::: gfx/gl/GLContextFeatures.cpp @@ +446,5 @@ > + GLVersion::NONE, > + GLESVersion::NONE, > + GLContext::Extension_None, > + { > + GLContext::ARB_shader_texture_lod, I believe this is wrong. Following the pattern it should be: ... GLContext::ARB_shader_texture_lod, { GLContext::EXT_shader_texture_lod, GLContext::Extensions_End } ... (But since the extension doesn't add functions to the C++ API, it's probably not an error, but I'd like it to keep the pattern of the other checks.)

Comment on attachment 8561303 [details] [diff] [review] ARB_shader_texture_lod support to implement WebGL EXT_shader_texture_lod. r=jgilbert Review of attachment 8561303 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.h @@ +122,1 @@ > sampler_objects, TBH, I generally sort case-insensitively. Since sorting is generally about making is easy to find things, it's easier to just remember one criteria (as a human) rather than two (a-z order, uppercase before lowercase). That said, "shader" goes after "sampler", but before "standard". ::: gfx/gl/GLContextFeatures.cpp @@ +446,5 @@ > + GLVersion::NONE, > + GLESVersion::NONE, > + GLContext::Extension_None, > + { > + GLContext::ARB_shader_texture_lod, So technically, it doesn't matter, since this exposes no C++ entrypoints. However, conceptually it should be in the extension list, as the GLSL functions it exposes are ARB-suffixed. I would leave this in the extension list.

Comment on attachment 8561303 [details] [diff] [review] ARB_shader_texture_lod support to implement WebGL EXT_shader_texture_lod. r=jgilbert Review of attachment 8561303 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextFeatures.cpp @@ +443,5 @@ > }, > { > + "shader_texture_lod", > + GLVersion::NONE, > + GLESVersion::NONE, I checked on when this went core, and this functionality is supported by GL3+ and GLES3+. However, the function signatures are different. It looks like ANGLE's translating should cover us, though.

This extension is still missing from the Nightly builds. Is there any possibility of exposing it soon? I'm having to maintain two shaders one with and without the extension, but the semantics only translate for LOD 0. Firefox OSX is the only browser omitting this extension.

Any roadmap to ship this feature? Another example that still fails on OSX http://vorg.github.io/pex/examples/glu.TextureCube.lod/ - there is a slider to manually select cubemap mipmap level

Dan, can you just take this bug to the finish line? I have no idea what actually needs to be done before this lands, and what are stylistic comments.

Landing this would potentially help out ThreeJS and other libraries that are implementing a roughness factor for physically based lighting. Please see this ThreeJS issue for details on how they are having to waste texture units to work around lack of the extension: https://github.com/mrdoob/three.js/issues/5847

We can base our test on this: https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/ext-shader-texture-lod.html?webglVersion=1&quiet=0

Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert

Bug 1111689: Part 2 - Update ext-shader-texture-lod WebGL conformance test - Cherry picked minimal changes to support updated ext-shader-texture-lod webgl conformance test.

The Part 1 patch hasn't changed since last r+'ed, except being rebased

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert https://reviewboard.mozilla.org/r/18713/#review17747 ::: dom/canvas/WebGLContextExtensions.cpp:119 (Diff revision 2) > - return gl->IsExtensionSupported(gl::GLContext::EXT_shader_texture_lod); > + return gl->IsExtensionSupported(gl::GLContext::EXT_shader_texture_lod) || > + gl->IsExtensionSupported(gl::GLContext::ARB_shader_texture_lod); Shouldn't you just use the GLFeature you added here?

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so https://reviewboard.mozilla.org/r/18721/#review17749

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so Bug 1111689: Part 2 - Update ext-shader-texture-lod WebGL conformance test,r=jgilbert - Cherry picked minimal changes to support updated ext-shader-texture-lod webgl conformance test.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0a637e8000 https://hg.mozilla.org/integration/mozilla-inbound/rev/909b9b147e71

Those ASan mochitest-gl leaks on your try push? They weren't a coincidence, they were from these patches. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c2577d136c72

Thanks Phil, I will address the leaks and do another try run to verify.

I have reproduced two leaks by running an ASAN build locally in Linux: TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at __interceptor_strdup, /usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so, js::frontend::BytecodeEmitter::bindNameToSlotHelper TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at __interceptor_strdup, /usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so, js::Invoke These occurred with the "part 2" patch for this bug, but not with just the "part 1" patch for this bug applied. Could you please confirm that these are the ones that were not just a co-incidence?

Doesn't sound entirely and exactly like the leak in automation, https://treeherder.mozilla.org/logviewer.html#?job_id=14394183&repo=mozilla-inbound, but then, I don't know anything about ASan other than how to tell whether it is green or orange.

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so Bug 1111689: Part 2 - Update ext-shader-texture-lod WebGL conformance test,r=jgilbert - Cherry picked minimal changes to support updated ext-shader-texture-lod webgl conformance test.

(In reply to :kip (Kearwood Gilbert) from comment #40) I have updated the mochitest and minified the related webgl-test-utils.js changes further. In my local linux-asan builds, this has eliminated at least two issues found by the Leak Sanitizer: TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at __interceptor_strdup, /usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so, js::frontend::BytecodeEmitter::bindNameToSlotHelper TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at __interceptor_strdup, /usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so, js::Invoke I have pushed to try to run the mochitests on all platforms to ensure there are no regressions and that the try server sees the same result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac7ee0b8ee4c

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so Bug 1111689: Part 2 - Update ext-shader-texture-lod WebGL conformance test,r=jgilbert - Cherry picked minimal changes to support updated ext-shader-texture-lod webgl conformance test.

(In reply to :kip (Kearwood Gilbert) from comment #41) > (In reply to :kip (Kearwood Gilbert) from comment #40) > > I have updated the mochitest and minified the related webgl-test-utils.js > changes further. In my local linux-asan builds, this has eliminated at > least two issues found by the Leak Sanitizer: > > > TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at __interceptor_strdup, > /usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so, > js::frontend::BytecodeEmitter::bindNameToSlotHelper > TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at __interceptor_strdup, > /usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so, js::Invoke > > I have pushed to try to run the mochitests on all platforms to ensure there > are no regressions and that the try server sees the same result: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac7ee0b8ee4c That try run was inadvertently running only reftests. Please see this one for the mochitests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be7814a807f8

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18713/diff/5-6/

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18721/diff/4-5/

I have un-bitrotted these patches and pushed to try to see what / if any leaks are still caused by the conformance tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18713/diff/6-7/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18721/diff/5-6/

Patches rebased

Comment on attachment 8698584 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Since MozReview can't seem to mark this r+, I'll do it myself.

Thanks Jeff. Sorry to generate extra bugmail for you. I wished to update the patches without re-submitting for review.

Looks like the patches are r+. Did this land, though?

Thanks for bringing this to my attention, Milan, as it fell between the cracks over the holidays. The patches still apply -- I'll do a try run and if it still passes, then I can land this.

Windows builds did not launch in try push. Pushing again..

Please get this landed ASAP!

Wait.. this patch can't work. If ARB_shader_texture_lod is supported, the new functions have an ARB suffix. So at a minimum, the ANGLE shader translator will need to be updated to either rename, e.g. textureCubeLodEXT -> textureCubeLodARB, or to implement wrapper functions with the EXT suffix in a preamble that will call the ARB functions. Does the test actually pass on OSX, which is where we only have ARB_shader_texture_lod?

The test passes and I am able to use sites that except use the extension; however, there were potentially some leaks still occurring reported in the Linux Asan builds. My earlier update fixes some of the leaks but it is unclear if the others are still an issue: I could use help from someone familiar with the asan reports to look at the earlier try runs and see if it is something to be concerned with before landing

Yeah, it looks like it only accidentally works. We should make the feature depend on OpenGL >= 3.2, and not ARB_shader_texture_lod. ANGLE does *not* understand ARB_shader_texture_lod at all. It will either translate things to EXT_shader_texture_lod (if the target is OpenGL ES), or to the core profile calls if the target output is GLSL >= 1.3. So, "textureCubeGradEXT" is translated to "textureGrad", and not to "textureCubeGradARB" as it should be if ARB_shader_texture_lod was actually being used. It happens to work because the functions are core now, and things like textureGrad are available without explicitly enabling an extension in the shader. For the Linux ASAN issue -- it looks like a leak inside the mesa/dri glsl compiler. I think you add a suppression and move on.

Hmm.. maybe I'm wrong about ANGLE not emitting the ARB names (and I wrote this code even, I think). So, ignore the above, other than the ASAN note.. I think you can just suppress that and ignore it.

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18713/diff/7-8/

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18721/diff/6-7/

- Suppress ASAN leak reports due to leaks in the mesa/dri glsl compiler. Review commit: https://reviewboard.mozilla.org/r/41791/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41791/

I have rebased the patches and added libglsl.so to the ASAN exclusions. If the try push shows that we have caught the last of the ASAN failures and Vlad's okay with this way of suppressing the glsl compiler leaks, then I can land this.

The last try run failed with: "1090639 Only broken slaves running at too small a resolution hit test_conformance__attribs__gl-vertex-attrib-zero-issues.html | Unable to fetch WebGL rendering context for Canvas" This seems to indicate that the failure was not related to the patch, so I've pushed to try again.

Got another bug report from Epic about this - on OS X, Safari and Chrome do support WebGL EXT_shader_texture_lod extension but Firefox is missing out. Wrote down bug 1270435 since didn't notice this at first. This bug item relates to expanding the texture_lod support to cover also native GL systems that have GL_ARB_shader_texture_lod, which is case #1 from https://bugzilla.mozilla.org/show_bug.cgi?id=1270435#c0. After this lands, will this also cover case #2 from the same comment? Looking at my OS X laptop, it no longer lists GL_ARB_shader_texture_lod extension, since it's running with GL 4.1 core, so it will fall under case #2 in there. Kip, was the latest try green?

Comment on attachment 8733476 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so https://reviewboard.mozilla.org/r/41791/#review47553

I was waiting on a review for a patch that suppresses ASAN leaks caused by the GPU driver. I have changed the reviewer, so hopefully will get seen sooner. My last try push resulted in the errors in Comment 76, which seemed to indicate problems with the test nodes. I've pushed again hoping to see them cleared and to verify that this patch does not need rebasing. Otherwise, this patch is ready to land. I've left the NI on so I can check into your other question.

Cool, it's all r+ now.

Bug 1193526 updated the WebGL conformance tests, so it should not longer be needed to apply the 2nd patch, which cherry picks the ext-shader-texture-lod conformance test. I'm re-basing the other patches now.

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18713/diff/8-9/

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18721/diff/7-8/

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18713/diff/9-10/

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18721/diff/8-9/

Review commit: https://reviewboard.mozilla.org/r/51403/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51403/

Comment on attachment 8658891 [details] MozReview Request: Bug 1111689 - Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18713/diff/10-11/

Comment on attachment 8658903 [details] MozReview Request: Bug 1111689 - part 3: Suppress ASAN leak reports for libglsl.so Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18721/diff/9-10/

The WebGL ensure-exts mochitests appear to have failed in the last try push due to expecting the extension to be disabled. I have updated the ensure-exts mochitest and have pushed again.

(In reply to :kip (Kearwood Gilbert) from comment #90) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffba59e6be9a Please disregard this try push, it is not related to this bug (wrong patch-set applied during try push)

The last try push in MozReview shows that this patch is working for OSX, but the extension is not being exposed on the other platforms. Perhaps this is due to bug 1270435 (See Comment 77). @jgilbert - Would it make sense to update the ensure-exts mochitests to expect the extension only on OSX for now, then fix bug 1270435 after this lands?

(In reply to :kip (Kearwood Gilbert) from comment #92) > The last try push in MozReview shows that this patch is working for OSX, but > the extension is not being exposed on the other platforms. Perhaps this is > due to bug 1270435 (See Comment 77). > > @jgilbert - Would it make sense to update the ensure-exts mochitests to > expect the extension only on OSX for now, then fix bug 1270435 after this > lands? This patch regresses EXT_shader_texture_lod on Windows, which is more important than adding support to OSX. We don't care about having it ensured-exposed on all platforms per se, but we don't want to regress what we already have.

Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/967d519f5d08 Part 1: Let EXT_shader_texture_lod play when ARB_shader_texture_lod can play. r=jgilbert https://hg.mozilla.org/integration/mozilla-inbound/rev/cb93aa0fe394 Suppress ASAN leak reports for libglsl.so. - r=jgilbert https://hg.mozilla.org/integration/mozilla-inbound/rev/e912f451cae4 Remark failures.

The problem was the GLFeature definition was added in the same order as the declarations. The changes here are fine, just needed that fix and a unexpected-pass remark.

https://hg.mozilla.org/mozilla-central/rev/967d519f5d08 https://hg.mozilla.org/mozilla-central/rev/cb93aa0fe394 https://hg.mozilla.org/mozilla-central/rev/e912f451cae4

Developer release notes: https://developer.mozilla.org/en-US/Firefox/Releases/50#WebGL Reference docs: https://developer.mozilla.org/en-US/docs/Web/API/EXT_shader_texture_lod Updated support info: https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/Using_Extensions https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getSupportedExtensions https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getExtension