Bug 128985 - WebGL EXT_shader_texture_lod may be implemented
Summary: WebGL EXT_shader_texture_lod may be implemented
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://www.khronos.org/registry/webgl...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-18 11:29 PST by Florian Bösch
Modified: 2014-05-12 13:14 PDT (History)
14 users (show)

See Also:


Attachments
Patch (26.00 KB, patch)
2014-05-08 13:06 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (41.87 KB, patch)
2014-05-09 14:43 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (43.21 KB, patch)
2014-05-09 16:05 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Bösch 2014-02-18 11:29:45 PST
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.
Comment 1 Radar WebKit Bug Importer 2014-02-19 10:53:07 PST
<rdar://problem/16111396>
Comment 2 Alex Christensen 2014-05-08 13:06:21 PDT
Created attachment 231094 [details]
Patch
Comment 3 Alex Christensen 2014-05-08 13:09:42 PDT
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.
Comment 4 Brent Fulgham 2014-05-08 16:53:52 PDT
Is this already part of the ANGLE we have checked in?
Comment 5 Dean Jackson 2014-05-08 17:44:39 PDT
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?
Comment 6 Dean Jackson 2014-05-08 18:50:11 PDT
I think we also need to set EXT_shader_texture_lod in ShBuiltInResources when we initialise ANGLE in GraphicsContext3D
Comment 7 Alex Christensen 2014-05-09 10:54:46 PDT
(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?
Comment 8 Dean Jackson 2014-05-09 13:40:19 PDT
(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.
Comment 9 Dean Jackson 2014-05-09 13:43:18 PDT
> > (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
Comment 10 Alex Christensen 2014-05-09 14:43:49 PDT
Created attachment 231185 [details]
Patch
Comment 11 Alex Christensen 2014-05-09 14:47:46 PDT
Comment on attachment 231185 [details]
Patch

The test passes with or without the rest of the changes.
Comment 12 Alex Christensen 2014-05-09 16:05:26 PDT
Created attachment 231192 [details]
Patch
Comment 13 Alex Christensen 2014-05-09 16:09:28 PDT
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.
Comment 14 Dean Jackson 2014-05-09 17:33:26 PDT
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.
Comment 15 Alex Christensen 2014-05-12 10:50:32 PDT
(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.
Comment 16 Dean Jackson 2014-05-12 12:25:53 PDT
(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.
Comment 17 WebKit Commit Bot 2014-05-12 13:14:14 PDT
Comment on attachment 231192 [details]
Patch

Clearing flags on attachment: 231192

Committed r168639: <http://trac.webkit.org/changeset/168639>
Comment 18 WebKit Commit Bot 2014-05-12 13:14:21 PDT
All reviewed patches have been landed.  Closing bug.