The default bug view has changed. See this FAQ.

Implement WebGL WEBGL_depth_texture extension

RESOLVED FIXED in mozilla17

Status

()

Core
Canvas: WebGL
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jgilbert, Assigned: Alexander Boldyrev)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jgilbert][lang=c++] webgl-extension, URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
No longer blocks: 728319
Whiteboard: [mentor=jgilbert][lang=c++] → [mentor=jgilbert][lang=c++] webgl-extension
(Assignee)

Comment 1

5 years ago
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.
Yep. Have a look at the thread "Restricting WebGL exposure of OES_depth_texture" on the public_webgl mailing list (archives online).
(Reporter)

Updated

5 years ago
Summary: Implement WebGL OES_depth_texture extension → Implement WebGL WEBGL_depth_texture extension
(Reporter)

Updated

5 years ago
Assignee: nobody → boldir
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 640645 [details] [diff] [review]
patch: WEBGL_depth_texture implementation
Attachment #640645 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #640645 - Attachment description: patch: WEBGL_depth_texture implemention → patch: WEBGL_depth_texture implementation
Attachment #640645 - Flags: review? → review?(jgilbert)
(Reporter)

Comment 4

5 years ago
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.
Attachment #640645 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
> ::: 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?
(Reporter)

Comment 7

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
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.
(Reporter)

Comment 9

5 years ago
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!
(Assignee)

Comment 10

5 years ago
Created attachment 644739 [details] [diff] [review]
patch (revision 2): WEBGL_depth_texture implementation
Attachment #640645 - Attachment is obsolete: true
Attachment #644739 - Flags: review?(jgilbert)
(Reporter)

Comment 11

5 years ago
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.
Attachment #644739 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 645734 [details] [diff] [review]
(on top of other patch) small fixes
Attachment #645734 - Flags: review?(jgilbert)
(Reporter)

Comment 13

5 years ago
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?
Attachment #645734 - Flags: review?(jgilbert) → review+
If you commit, make sure to use -u "name <email>"
(Assignee)

Comment 15

5 years ago
(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?
(Reporter)

Comment 16

5 years ago
(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.
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=jgilbert][lang=c++] webgl-extension → [mentor=jgilbert][lang=c++] webgl-extension [rplus]
(Reporter)

Comment 17

5 years ago
Try running: https://tbpl.mozilla.org/?tree=Try&rev=ba60c0eecb24
(Reporter)

Comment 18

5 years ago
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
(Reporter)

Comment 19

5 years ago
Created attachment 651599 [details] [diff] [review]
to checkin

Ok, this passes try.
Attachment #651599 - Flags: review+
(Reporter)

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/376a20e68396
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=jgilbert][lang=c++] webgl-extension [rplus] → [mentor=jgilbert][lang=c++] webgl-extension
https://hg.mozilla.org/mozilla-central/rev/376a20e68396
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 783914
You need to log in before you can comment on or make changes to this bug.