Bug 128985 – WebGL EXT_shader_texture_lod may be implemented

• Component: WebGL
• Status: RESOLVED FIXED

[Florian Bösch]

The WebGL Extension EXT_shader_texture_lod has been moved to draft: http://www.khronos.org/registry/webgl/extensions/EXT_shader_texture_lod/

An angle patch has been submitted: https://code.google.com/p/angleproject/issues/detail?id=551

The chrome ticket: https://code.google.com/p/chromium/issues/detail?id=344583
The firefox ticket and patch: https://bugzilla.mozilla.org/show_bug.cgi?id=965848

Vendors are encouraged to consider implementation of this extension.

[Radar WebKit Bug Importer]

<rdar://problem/16111396>

[Alex Christensen]

Created attachment 231094 [details]
Patch

[Alex Christensen]

It would be nice to have a test case like ext-shader-texture-lod.html from https://bug965848.bugzilla.mozilla.org/attachment.cgi?id=8367994 but I'm not sure where it came from.  Chromium has no new test in https://src.chromium.org/viewvc/blink?view=rev&revision=171465 but maybe they should.  I don't think we should add this without a test, but maybe whoever reviews this would know more about where webgl tests come from than I do.

[Brent Fulgham]

Is this already part of the ANGLE we have checked in?

[Dean Jackson]

Comment on attachment 231094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231094&action=review

We definitely need a test. There is nothing in the official testsuite :(
This means you'll have to write something from scratch. For now, copy the existing approach in LayoutTests/webgl and we'll submit the test to khronos when done.

And yes, we have the ANGLE fixes.

> Source/WebCore/ChangeLog:12
> +        * WebCore.vcxproj/WebCore.vcxproj:

Don't we also add to the .filters file?

> Source/WebCore/CMakeLists.txt:2941
>  list(APPEND WebCore_SOURCES
> +    platform/text/TextCodecICU.cpp
> +    platform/text/TextEncodingDetectorICU.cpp
>      platform/text/icu/UTextProvider.cpp
>      platform/text/icu/UTextProviderLatin1.cpp
>      platform/text/icu/UTextProviderUTF16.cpp
> -    platform/text/TextCodecICU.cpp
> -    platform/text/TextEncodingDetectorICU.cpp
>  )

Was this intentional?

[Dean Jackson]

I think we also need to set EXT_shader_texture_lod in ShBuiltInResources when we initialise ANGLE in GraphicsContext3D

[Alex Christensen]

(In reply to comment #5)
> This means you'll have to write something from scratch.
Do you think adapting ext-shader-texture-lod.html from https://bug965848.bugzilla.mozilla.org/attachment.cgi?id=8367994 would work?  Maybe that should be put into the official test suite.

(In reply to comment #6)
> I think we also need to set EXT_shader_texture_lod in ShBuiltInResources when we initialise ANGLE in GraphicsContext3D
Do you mean in Extensions3DOpenGLCommon::ensureEnabled?

[Dean Jackson]

(In reply to comment #7)
> (In reply to comment #5)
> > This means you'll have to write something from scratch.
> Do you think adapting ext-shader-texture-lod.html from https://bug965848.bugzilla.mozilla.org/attachment.cgi?id=8367994 would work?  Maybe that should be put into the official test suite.

Cool, yes. We should use that. And it should go into the official test suite. I'll ping mozilla.


> 
> (In reply to comment #6)
> > I think we also need to set EXT_shader_texture_lod in ShBuiltInResources when we initialise ANGLE in GraphicsContext3D
> Do you mean in Extensions3DOpenGLCommon::ensureEnabled?

No. I think that we need to pass in a flag to ANGLE to tell it that it can use the LOD built-ins in the Shader validator, otherwise the GLSL will be rejected.

See, ShaderLang.h

    // Extensions.
    // Set to 1 to enable the extension, else 0.
    int OES_standard_derivatives;
    int OES_EGL_image_external;
    int ARB_texture_rectangle;
    int EXT_draw_buffers;
    int EXT_frag_depth;
    int EXT_shader_texture_lod;

in ShBuiltInResources. We just need to set that when we create the validator.

[Dean Jackson]

> > (In reply to comment #6)
> > > I think we also need to set EXT_shader_texture_lod in ShBuiltInResources when we initialise ANGLE in GraphicsContext3D
> > Do you mean in Extensions3DOpenGLCommon::ensureEnabled?
> 
> No. I think that we need to pass in a flag to ANGLE to tell it that it can use the LOD built-ins in the Shader validator, otherwise the GLSL will be rejected.
> 
> See, ShaderLang.h
> 
>     // Extensions.
>     // Set to 1 to enable the extension, else 0.
>     int OES_standard_derivatives;
>     int OES_EGL_image_external;
>     int ARB_texture_rectangle;
>     int EXT_draw_buffers;
>     int EXT_frag_depth;
>     int EXT_shader_texture_lod;
> 
> in ShBuiltInResources. We just need to set that when we create the validator.

Duh! Yes, that *is* where it should go, sorry. Extensions3DOpenGLCommon::ensureEnabled

[Alex Christensen]

Created attachment 231185 [details]
Patch

[Alex Christensen]

Comment on attachment 231185 [details]
Patch

The test passes with or without the rest of the changes.

[Alex Christensen]

Created attachment 231192 [details]
Patch

[Alex Christensen]

I had to use GL_ARB_shader_texture_lod instead of GL_EXT_shader_texture_lod because GL_ARB_shader_texture_lod was in the string returned by glGetString(GL_EXTENSIONS).  Now it works and passes the test.

(In reply to comment #5)
>> Source/WebCore/CMakeLists.txt:2941
>>  list(APPEND WebCore_SOURCES
>> +    platform/text/TextCodecICU.cpp
>> +    platform/text/TextEncodingDetectorICU.cpp
>>      platform/text/icu/UTextProvider.cpp
>>      platform/text/icu/UTextProviderLatin1.cpp
>>      platform/text/icu/UTextProviderUTF16.cpp
>> -    platform/text/TextCodecICU.cpp
>> -    platform/text/TextEncodingDetectorICU.cpp
>>  )
> Was this intentional?
Yes.  The style bot got mad about alphabetization without me doing that.

[Dean Jackson]

Comment on attachment 231192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231192&action=review

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:140
> +        // Enable support in ANGLE (if not enabled already)

Nit: missing period.

> LayoutTests/ChangeLog:14
> +        * webgl/conformance/extensions/ext-shader-texture-lod-expected.txt: Added.
> +        * webgl/conformance/extensions/ext-shader-texture-lod.html: Added.
> +        * webgl/resources/webgl_test_files/conformance/extensions/ext-shader-texture-lod.html: Added.

Since this test isn't (yet) part of the official test suite, I think it should go in fast/canvas/webgl.

Otherwise we're likely to overwrite it if merge tests from Khronos.

[Alex Christensen]

(In reply to comment #14)
> Since this test isn't (yet) part of the official test suite, I think it should go in fast/canvas/webgl.
Putting it in there makes it not work because of differences in the included files (webgl-test.js, etc.).  Should I try to squeeze it in there anyway?  I think once this becomes part of the official test suite, it won't differ much from what is included in this patch, except the copyright will be Khronos instead of chromium.

[Dean Jackson]

(In reply to comment #15)
> (In reply to comment #14)
> > Since this test isn't (yet) part of the official test suite, I think it should go in fast/canvas/webgl.
> Putting it in there makes it not work because of differences in the included files (webgl-test.js, etc.).  Should I try to squeeze it in there anyway?  I think once this becomes part of the official test suite, it won't differ much from what is included in this patch, except the copyright will be Khronos instead of chromium.

Sure, no problem.

[WebKit Commit Bot]

Comment on attachment 231192 [details]
Patch

Clearing flags on attachment: 231192

Committed r168639: <http://trac.webkit.org/changeset/168639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.