Closed Bug 1170842 Opened 5 years ago Closed 5 years ago

WebGL 2 - Finish implementing framebuffers

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: djg, Assigned: djg)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 4 obsolete files)

WebGLContextFramebuffers.cpp contains a couple of MOZ_CRASH("Not Implemented."):

void WebGL2Context::FramebufferTextureLayer(...)
void WebGL2Context::GetInternalformatParameter(...)
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
Attachment #8622887 - Flags: review?(jgilbert)
Attachment #8622886 - Attachment is obsolete: true
Attachment #8622886 - Flags: review?(jgilbert)
Attachment #8622916 - Flags: review?(jgilbert)
Attachment #8622887 - Attachment is obsolete: true
Attachment #8622887 - Flags: review?(jgilbert)
Attachment #8622918 - Flags: review?(jgilbert)
Comment on attachment 8622916 [details] [diff] [review]
Part 1: Sort out ARB_framebuffer_object symbol queries.

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

::: gfx/gl/GLContext.cpp
@@ +780,2 @@
>  
> +            bool ARB_framebuffer_object_supported = true;

Please don't start local vars with caps. `hasARBFramebufferObject` should be fine.

::: gfx/gl/GLContext.h
@@ +446,2 @@
>          NV_half_float,
> +        NV_framebuffer_blit,

Keep this alphabetized!

::: gfx/gl/GLContextFeatures.cpp
@@ +813,5 @@
>  void
> +GLContext::MarkSupported(GLFeature feature)
> +{
> +    mAvailableFeatures[size_t(feature)] = true;
> +    // TODO: Should this enable the extension too?

No, extensions should stay as is.
Attachment #8622916 - Flags: review?(jgilbert) → review+
Comment on attachment 8622918 [details] [diff] [review]
Part 2: Implement FramebufferTextureLayer.

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

I'm concerned about the -1 handling. Otherwise just nits.

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +332,5 @@
> +static bool
> +ValidateTextureLayerAttachment(GLenum attachment)
> +{
> +    if (LOCAL_GL_COLOR_ATTACHMENT0 < attachment &&
> +        attachment <= LOCAL_GL_COLOR_ATTACHMENT15)

I guess? Really it needs to be less than the queryable max color attachment limit.

@@ +386,5 @@
> +            break;
> +
> +        default:
> +            return ErrorInvalidOperation("framebufferTextureLayer: texture must be an "
> +                                         "existing 3D texture, or a 2D texture array.");

This is disappointing to me, but is what the spec says.

::: dom/canvas/WebGL2ContextRenderbuffers.cpp
@@ +21,5 @@
> +        return;
> +
> +    if (target != LOCAL_GL_RENDERBUFFER) {
> +        return ErrorInvalidEnumInfo("getInternalfomratParameter: target must be "
> +                                    "RENDERBUFFER. Was:", target);

I'd really like to stop returning void func calls into void.

@@ +36,5 @@
> +
> +    GLint* samples = nullptr;
> +    GLint sampleCount = 0;
> +    gl->fGetInternalformativ(LOCAL_GL_RENDERBUFFER, internalformat,
> +                             LOCAL_GL_NUM_SAMPLE_COUNTS, 1, &sampleCount);

We should probably just return The Right Thing without asking the driver.

::: dom/canvas/WebGLContext.h
@@ +1171,5 @@
>      int32_t mGLMaxDrawBuffers;
>      uint32_t  mGLMaxTransformFeedbackSeparateAttribs;
>      GLuint  mGLMaxUniformBufferBindings;
>      GLsizei mGLMaxSamples;
> +    GLint   mGLMax3DTextureSize;

Limits should really be unsigned.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +20,5 @@
>      : mFB(fb)
>      , mAttachmentPoint(attachmentPoint)
>      , mTexImageTarget(LOCAL_GL_NONE)
> +    , mTexImageLayer(-1)
> +    , mTexImageLevel(-1)

Just leave these as undefined. We should only consider them defined when mTexturePtr is truthy, and completely ignore them otherwise.

@@ +133,5 @@
>      mTexturePtr = tex;
>      mRenderbufferPtr = nullptr;
>      mTexImageTarget = target;
>      mTexImageLevel = level;
> +    mTexImageLayer = -1;

Shouldn't this default to 0? And shouldn't this func really just call into SetTexImageLayer below, being a subset of it?

@@ +166,5 @@
>      UnmarkAttachment(*this);
>  
>      mTexturePtr = nullptr;
>      mRenderbufferPtr = rb;
> +    mTexImageTarget = LOCAL_GL_NONE;

Don't bother clearing these here.
Attachment #8622918 - Flags: review?(jgilbert) → review-
Attachment #8622888 - Flags: review?(jgilbert) → review+
Attachment #8622889 - Flags: review?(jgilbert) → review+
Attachment #8622916 - Attachment is obsolete: true
Attachment #8624002 - Flags: review?(jgilbert)
Attachment #8622918 - Attachment is obsolete: true
Comment on attachment 8624002 [details] [diff] [review]
Part 1: Sort out ARB_framebuffer_object symbol queries.

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

::: gfx/gl/GLContext.cpp
@@ +57,5 @@
>  
>  
>  #define MAX_SYMBOL_LENGTH 128
>  #define MAX_SYMBOL_NAMES 5
> +#define _STR(x) #x

Not really necessary.

@@ +839,5 @@
> +            if (IsExtensionSupported(GLContext::ARB_geometry_shader4) ||
> +                IsExtensionSupported(GLContext::NV_geometry_program4))
> +            {
> +                SymLoadStruct extSymbols[] = {
> +                    EXT_SYMBOL2(FramebufferTextureLayer, ARB, EXT),

Huh, the NV ext exposes EXT suffixed funcs?

::: gfx/gl/GLContextFeatures.cpp
@@ +825,5 @@
>  void
> +GLContext::MarkSupported(GLFeature feature)
> +{
> +    mAvailableFeatures[size_t(feature)] = true;
> +    // TODO: Should this enable the extension too?

I don't think it should.
Attachment #8624002 - Flags: review?(jgilbert) → review+
Comment on attachment 8624003 [details] [diff] [review]
Part 2: Implement FramebufferTextureLayer.

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

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +378,5 @@
> +            }
> +            break;
> +
> +        case LOCAL_GL_TEXTURE_2D_ARRAY:
> +            if ((GLuint) layer >= mGLMaxArrayTextureLayers) {

Do we delay checking against the actual TexImage until the FB completeness check?

@@ +397,5 @@
> +    WebGLFramebuffer* fb;
> +    switch (target) {
> +    case LOCAL_GL_FRAMEBUFFER:
> +    case LOCAL_GL_DRAW_FRAMEBUFFER:
> +        fb = mBoundDrawFramebuffer;

We should really make this stuff an out-var from the target validation function.

::: dom/canvas/WebGL2ContextRenderbuffers.cpp
@@ +48,5 @@
> +    if (!obj) {
> +        rv = NS_ERROR_OUT_OF_MEMORY;
> +    }
> +
> +    delete[] samples;

Just use a UniquePtr.
Attachment #8624003 - Flags: review?(jgilbert) → review+
Comment on attachment 8624003 [details] [diff] [review]
Part 2: Implement FramebufferTextureLayer.

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

Yeah, we need to update our FB attachment completeness.
Attachment #8624003 - Flags: review+ → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> Comment on attachment 8624002 [details] [diff] [review]
> ::: gfx/gl/GLContext.cpp
> > +#define _STR(x) #x
> 
> Not really necessary.

All the macros aren't, but I'd like to clean up all the struct setting. Is there a standard "stringify" macro? Otherwise, it's needed to get the macro turned into a string to then concatenate...

Unless... I use string pasting, like #glname #suffix. Not sure if that works.
(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> Huh, the NV ext exposes EXT suffixed funcs?

Yeeeeep. https://www.opengl.org/registry/specs/NV/geometry_program4.txt
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 8624003 [details] [diff] [review]
> Part 2: Implement FramebufferTextureLayer.
> 
> Review of attachment 8624003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, we need to update our FB attachment completeness.

OK, we need talk about this. I've been staring that the completeness check code and I'm not sure exactly what needs to be updated.
Spoke with :jgilbert. Going to land this so we can then get Jeff's texture refactoring landed, which can then be used to make texture layers more robust.

Green try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1674d61a0de
Comment on attachment 8622889 [details] [diff] [review]
Part 4: Implement GetInternalformatParameter.

Olli, I moved some webidl function and added [throw] to getInternalformatParameter because the function allocates an Int32Array to return the result, which can fail with NS_ERROR_OUT_OF_MEMORY.
Attachment #8622889 - Flags: review?(bugs)
Attachment #8622889 - Flags: review?(bugs) → review+
Duplicate of this bug: 1225610
You need to log in before you can comment on or make changes to this bug.