738866 - Implement WebGL WEBGL_depth_texture extension

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

[Kelsey Gilbert [:jgilbert]]

The extension draft is here:
http://www.khronos.org/registry/webgl/extensions/OES_depth_texture/

An overview of how WebGL extensions are added is here: https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions

Comment 1

[Alexander Boldyrev]

Hi, I'd like to work on this bug. 
But I couldn't find OES_depth_texture extension specification (404 error). There is only WEBGL_depth_texture:
http://www.khronos.org/registry/webgl/extensions/WEBGL_depth_texture/
As I understood, extension was renamed.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Yep. Have a look at the thread "Restricting WebGL exposure of OES_depth_texture" on the public_webgl mailing list (archives online).

Comment 3

[Alexander Boldyrev]

Attachment: patch: WEBGL_depth_texture implementation

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 640645 [details] [diff] [review]
patch: WEBGL_depth_texture implementation

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

First off, thanks for the patch!

A number of the issues are just style nits, but there are a couple real issues, and a few questions.

Please respond to or fix each one, but if you have any questions, just ask. :)
When you think you have it fixed up again, just attach a new patch and tag me for review again.

::: content/canvas/src/Makefile.in
@@ +50,5 @@
>  	WebGLContextValidate.cpp \
>  	WebGLExtensionStandardDerivatives.cpp \
>  	WebGLExtensionTextureFilterAnisotropic.cpp \
>  	WebGLExtensionLoseContext.cpp \
> +  WebGLExtensionDepthTexture.cpp \

Makefile.in appears to use tabs instead of 2-space indents. Unfortunately, this means we need to use tabs here too.

::: content/canvas/src/WebGLContext.h
@@ +2670,5 @@
> +            WebGLenum format = mTexturePtr->ImageInfoAt(0).Format();
> +            switch (mAttachmentPoint)
> +            {
> +                case LOCAL_GL_COLOR_ATTACHMENT0:
> +                    return true;

We need to check that the color attachment's format is color-renderable.

@@ +2677,5 @@
> +                case LOCAL_GL_DEPTH_STENCIL_ATTACHMENT:
> +                    return format == LOCAL_GL_DEPTH_STENCIL;
> +
> +                default:
> +                    NS_ABORT();

Use MOZ_NOT_REACHED(reason). Update the NS_ABORT() below to MOZ_NOT_REACHED while we're 'here'.

@@ -2781,5 @@
>              return;
>          }
>  
>          if (target != LOCAL_GL_FRAMEBUFFER)
> -            return mContext->ErrorInvalidEnumInfo("framebufferTexture2D: target", target);

Why was this changed?

::: content/canvas/src/WebGLContextGL.cpp
@@ +879,5 @@
>                                       bool sub)
>  {
> +    if (internalformat == LOCAL_GL_DEPTH_COMPONENT ||
> +        internalformat == LOCAL_GL_DEPTH_STENCIL)
> +        return ErrorInvalidOperation("copyTexSubImage2D: a base internal format of DEPTH_COMPONENT or DEPTH_STENCIL isn't supported");

This test should be in CopyTex(Sub)Image2D() below.

@@ +2123,5 @@
>      if (IsTextureFormatCompressed(format))
>          return ErrorInvalidOperation("generateMipmap: Texture data at level zero is compressed.");
>  
> +    if (IsExtensionEnabled(WEBGL_depth_texture) && 
> +        (format == DEPTH_COMPONENT || format == DEPTH_STENCIL))

Are you missing some LOCAL_GL_ prefixes here?

@@ +5589,5 @@
>              break;
> +        case LOCAL_GL_DEPTH_COMPONENT:
> +        case LOCAL_GL_DEPTH_STENCIL:
> +            if (IsExtensionEnabled(WEBGL_depth_texture)) {
> +                if (target != LOCAL_GL_TEXTURE_2D || data != NULL || level != 0)

Leave this logic for further down in the function. The goal in these texImage functions, since we have so much to test, is to have each block of tests test only one thing at a time, and to do so clearly.

Also, in general, the order for testing should be INVALID_ENUM, INVALID_VALUE, INVALID_OPERATION. Leave this testing block for just checking the |format| enum. Just put this set of tests in its own block below.

@@ +5832,5 @@
>              return ErrorInvalidValue("texSubImage2D: with level > 0, width and height must be powers of two");
>      }
>  
> +    if (IsExtensionEnabled(WEBGL_depth_texture) && 
> +        (format == DEPTH_COMPONENT || format == DEPTH_STENCIL)) {

For multi-line conditionals, prefer brace on the next line, instead of at the end here, for readability. Also, missing some more LOCAL_GL_ prefixes here?

@@ +6113,5 @@
> +                return WebGLTexelConversions::D16;
> +            case LOCAL_GL_UNSIGNED_INT:
> +                return WebGLTexelConversions::D32;
> +            default:
> +                NS_ABORT_IF_FALSE(false, "Coding mistake?! Should never reach this point.");

MOZ_NOT_REACHED

@@ +6117,5 @@
> +                NS_ABORT_IF_FALSE(false, "Coding mistake?! Should never reach this point.");
> +                return WebGLTexelConversions::BadFormat;
> +        }
> +    } else if (format == LOCAL_GL_DEPTH_STENCIL) {
> +        if (type != LOCAL_GL_UNSIGNED_INT_24_8_WEBGL)

Prefer a similar style to the switch statement above, where if we *are* the right format, return the enum we need. Do our not-reached thing afterwards.

@@ +6119,5 @@
> +        }
> +    } else if (format == LOCAL_GL_DEPTH_STENCIL) {
> +        if (type != LOCAL_GL_UNSIGNED_INT_24_8_WEBGL)
> +        {
> +            NS_ABORT_IF_FALSE(false, "Coding mistake?! Should never reach this point.");

MOZ_NOT_REACHED

@@ +6192,5 @@
> +            return LOCAL_GL_DEPTH_COMPONENT32;
> +    } else if (format == LOCAL_GL_DEPTH_STENCIL) {
> +        if (type == LOCAL_GL_UNSIGNED_INT_24_8_WEBGL)
> +            return LOCAL_GL_DEPTH24_STENCIL8;
> +    }

This block could use some whitespace. For ifs that return, it's fine to drop the 'else if' chaining to improve readability.

::: content/canvas/test/webgl/conformance/extensions/webgl-depth-texture.html
@@ +28,5 @@
> +<html>
> +<head>
> +<meta charset="utf-8">
> +<link rel="stylesheet" href="../../resources/js-test-style.css"/>
> +<script src="../../resources/desktop-gl-constants.js" type="text/javascript"></script>

This line doesn't appear to be in the Khronos test. Did you add it? If so, why?

::: gfx/gl/GLContext.h
@@ +1508,5 @@
>          EXT_robustness,
>          ARB_sync,
>          OES_EGL_image,
>          OES_EGL_sync,
> +        ARB_depth_texture,

ARB_depth_texture was moved to core in GL 1.4, so no need to check for it.

::: gfx/gl/GLContextProviderOSMesa.cpp
@@ +177,5 @@
>  
>          // OSMesa's different from the other GL providers, it renders to an image surface, not to a pbuffer
>          sOSMesaLibrary.fPixelStore(OSMESA_Y_UP, 0);
>  
> +        return InitWithPrefix("mgl", true);

What is this change for?

::: gfx/gl/GLDefs.h
@@ +3267,5 @@
>  #define LOCAL_EGL_CONDITION_SATISFIED         0x30F6
>  #define LOCAL_EGL_SUCCESS                     0x3000
>  
> +// WEBGL_depth_texture
> +#define LOCAL_GL_UNSIGNED_INT_24_8_WEBGL      0x84FA

We should use LOCAL_GL_UNSIGNED_INT_24_8_EXT internally. UNSIGNED_INT_24_8_WEBGL is only for WebGL.

Comment 5

[Alexander Boldyrev]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> @@ -2781,5 @@
> >              return;
> >          }
> >  
> >          if (target != LOCAL_GL_FRAMEBUFFER)
> > -            return mContext->ErrorInvalidEnumInfo("framebufferTexture2D: target", target);
> 
> Why was this changed?

When the extension is enabled, "framebufferTexture2D" is extended to accept the target parameter DEPTH_ATTACHMENT and DEPTH_STENCIL_ATTACHMENT (from specification). See the new block that was added instead of this line.

>::: gfx/gl/GLContextProviderOSMesa.cpp
>@@ +177,5 @@
>>  
>>          // OSMesa's different from the other GL providers, it renders to an image surface, not to a pbuffer
>>          sOSMesaLibrary.fPixelStore(OSMESA_Y_UP, 0);
>>  
>> +        return InitWithPrefix("mgl", true);
>
>What is this change for?
>
>::: content/canvas/test/webgl/conformance/extensions/webgl-depth-texture.html
>@@ +28,5 @@
>> +<html>
>> +<head>
>> +<meta charset="utf-8">
>> +<link rel="stylesheet" href="../../resources/js-test-style.css"/>
>> +<script src="../../resources/desktop-gl-constants.js" type="text/javascript">></script>
>
>This line doesn't appear to be in the Khronos test. Did you add it? If so, why?

Forgot revert it. Sorry, my mistake.


About Khronos test: it doesn't work in the current copy of webgl conformance test (works fine in Khronos repo).
>FAIL successfullyParsed should be true. Threw exception ReferenceError: successfullyParsed is not defined
Should I do something with that? I think it fails because of old version of conformance test.

Comment 6

[Alexander Boldyrev]

> ::: gfx/gl/GLDefs.h
> @@ +3267,5 @@
> >  #define LOCAL_EGL_CONDITION_SATISFIED         0x30F6
> >  #define LOCAL_EGL_SUCCESS                     0x3000
> >  
> > +// WEBGL_depth_texture
> > +#define LOCAL_GL_UNSIGNED_INT_24_8_WEBGL      0x84FA
> 
> We should use LOCAL_GL_UNSIGNED_INT_24_8_EXT internally.
> UNSIGNED_INT_24_8_WEBGL is only for WebGL.

In other words, no UNSIGNED_INT_24_8_WEBGL in C++ code?

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to Alexander Boldyrev from comment #6)
> > ::: gfx/gl/GLDefs.h
> > @@ +3267,5 @@
> > >  #define LOCAL_EGL_CONDITION_SATISFIED         0x30F6
> > >  #define LOCAL_EGL_SUCCESS                     0x3000
> > >  
> > > +// WEBGL_depth_texture
> > > +#define LOCAL_GL_UNSIGNED_INT_24_8_WEBGL      0x84FA
> > 
> > We should use LOCAL_GL_UNSIGNED_INT_24_8_EXT internally.
> > UNSIGNED_INT_24_8_WEBGL is only for WebGL.
> 
> In other words, no UNSIGNED_INT_24_8_WEBGL in C++ code?

Yep.

(In reply to Alexander Boldyrev from comment #5)
> (In reply to Jeff Gilbert [:jgilbert] from comment #4)
> > @@ -2781,5 @@
> > >              return;
> > >          }
> > >  
> > >          if (target != LOCAL_GL_FRAMEBUFFER)
> > > -            return mContext->ErrorInvalidEnumInfo("framebufferTexture2D: target", target);
> > 
> > Why was this changed?
> 
> When the extension is enabled, "framebufferTexture2D" is extended to accept
> the target parameter DEPTH_ATTACHMENT and DEPTH_STENCIL_ATTACHMENT (from
> specification). See the new block that was added instead of this line.

Hah, so actually yes, the spec does say that. Unfortunately, the spec should almost certainly say 'attachment' instead of 'target' there. I'll email the webgl list to get this clarified/fixed. The call should look like framebufferTexture2D(FRAMEBUFFER, DEPTH_ATTACHMENT, TEXTURE_2D, texObject, 0).

> >::: gfx/gl/GLContextProviderOSMesa.cpp
> >@@ +177,5 @@
> >>  
> >>          // OSMesa's different from the other GL providers, it renders to an image surface, not to a pbuffer
> >>          sOSMesaLibrary.fPixelStore(OSMESA_Y_UP, 0);
> >>  
> >> +        return InitWithPrefix("mgl", true);
> >
> >What is this change for?
> >
> >::: content/canvas/test/webgl/conformance/extensions/webgl-depth-texture.html
> >@@ +28,5 @@
> >> +<html>
> >> +<head>
> >> +<meta charset="utf-8">
> >> +<link rel="stylesheet" href="../../resources/js-test-style.css"/>
> >> +<script src="../../resources/desktop-gl-constants.js" type="text/javascript">></script>
> >
> >This line doesn't appear to be in the Khronos test. Did you add it? If so, why?
> 
> Forgot revert it. Sorry, my mistake.
> 
> 
> About Khronos test: it doesn't work in the current copy of webgl conformance
> test (works fine in Khronos repo).
> >FAIL successfullyParsed should be true. Threw exception ReferenceError: successfullyParsed is not defined
> Should I do something with that? I think it fails because of old version of
> conformance test.

bjacob will know. I'll try to get ahold of him if he doesn't see this sooner.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 640645 [details] [diff] [review]
patch: WEBGL_depth_texture implementation

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

One more thing I almost forgot about.

::: content/canvas/src/WebGLContext.cpp
@@ +934,5 @@
>      {
>          if (IsExtensionSupported(WEBGL_compressed_texture_s3tc))
>              ext = WEBGL_compressed_texture_s3tc;
>      }
> +    else if (aName.Equals(NS_LITERAL_STRING("WEBGL_depth_texture"),

This should actually be "MOZ_WEBGL_depth_texture", since WEBGL_depth_texture is only a draft extension, at the moment.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 640645 [details] [diff] [review]
patch: WEBGL_depth_texture implementation

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

::: content/canvas/src/WebGLContext.cpp
@@ +1537,5 @@
>          arr.AppendElement(NS_LITERAL_STRING("MOZ_WEBGL_lose_context"));
>      if (IsExtensionSupported(WEBGL_compressed_texture_s3tc))
>          arr.AppendElement(NS_LITERAL_STRING("MOZ_WEBGL_compressed_texture_s3tc"));
> +    if (IsExtensionSupported(WEBGL_depth_texture))
> +        arr.AppendElement(NS_LITERAL_STRING("WEBGL_depth_texture"));

This should also be MOZ_WEBGL_depth_texture, sorry!

Comment 10

[Alexander Boldyrev]

Attachment: patch (revision 2): WEBGL_depth_texture implementation

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 644739 [details] [diff] [review]
patch (revision 2): WEBGL_depth_texture implementation

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

Two nits. R+, but please upload a patch with these two things fixed.
Many thanks for this work. :)

::: content/canvas/src/WebGLContext.h
@@ +2682,5 @@
> +                    return format == LOCAL_GL_DEPTH_STENCIL;
> +
> +                default:
> +                    MOZ_NOT_REACHED("Invalid WebGL texture format?");
> +                    NS_ABORT();

NS_ABORT isn't necessary after MOZ_NOT_REACHED.

::: content/canvas/src/WebGLContextGL.cpp
@@ +6120,5 @@
> +            case LOCAL_GL_UNSIGNED_INT:
> +                return WebGLTexelConversions::D32;
> +            default:
> +                MOZ_NOT_REACHED("Invalid WebGL texture format/type?");
> +                NS_ABORT_IF_FALSE(false, "Coding mistake?! Should never reach this point.");

MOZ_NOT_REACHED will crash for us, so we don't need NS_ABORT_IF_FALSE.

Comment 12

[Alexander Boldyrev]

Attachment: (on top of other patch) small fixes

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 645734 [details] [diff] [review]
(on top of other patch) small fixes

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

Great, thanks. Shall I commit this for you?

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

If you commit, make sure to use -u "name <email>"

Comment 15

[Alexander Boldyrev]

(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> Comment on attachment 645734 [details] [diff] [review]
> (on top of other patch) small fixes
> 
> Review of attachment 645734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks. Shall I commit this for you?

Yes. 
Do I have the access?

Comment 16

[Kelsey Gilbert [:jgilbert]]

(In reply to Alexander Boldyrev from comment #15)
> (In reply to Jeff Gilbert [:jgilbert] from comment #13)
> > Comment on attachment 645734 [details] [diff] [review]
> > (on top of other patch) small fixes
> > 
> > Review of attachment 645734 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Great, thanks. Shall I commit this for you?
> 
> Yes. 
> Do I have the access?

Oops, missed this.
Not yet. If you're curious, info is here: http://www.mozilla.org/hacking/committer/

Basically, you start with level 1 access, which is enough to push to Try Server, and is pretty easy to get.

Generally, patches need to run on Try before being committed to make sure they don't break any builds or tests. I can do this for now, but if you plan on contributing more in the future, you'll want to be able to do this yourself.

Level 1 access is also required for getting level 3 access, which grants access to commit changes to Inbound, which becomes mozilla-central.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Try running: https://tbpl.mozilla.org/?tree=Try&rev=ba60c0eecb24

Comment 18

[Kelsey Gilbert [:jgilbert]]

Updated the wiki page for adding an extension, since there's a new Dom test in M3 which appears to check that we have no unknown global classes? Regardless, fixed that and a new Try is running and passing M3 here:
https://tbpl.mozilla.org/?tree=Try&rev=153ba888f4f6

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: to checkin

Ok, this passes try.

Comment 20

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/376a20e68396

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/376a20e68396