Fix BlitFramebuffer

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
mtseng
: review+
Details
58 bytes, text/x-review-board-request
mtseng
: review+
Details
58 bytes, text/x-review-board-request
mtseng
: review+
Details
58 bytes, text/x-review-board-request
mtseng
: review+
Details
58 bytes, text/x-review-board-request
mtseng
: review+
Details
58 bytes, text/x-review-board-request
mtseng
: review+
Details
58 bytes, text/x-review-board-request
mtseng
: review+
Details
58 bytes, text/x-review-board-request
mtseng
: review+
ethlin
: review+
jerry
: review+
Details
58 bytes, text/x-review-board-request
Details
(Assignee)

Description

2 years ago
In particular, blitFramebuffer doesn't handle feedback right now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8792681 [details]
Bug 1303879 - Validate attachments inside ScopedResolveTextures. -

https://reviewboard.mozilla.org/r/79590/#review78362

::: dom/canvas/WebGLContextDraw.cpp:498
(Diff revision 1)
>                          "%s, this will affect performance.",
>                          info,
>                          WebGLContext::EnumName(type));
>      }
>  
>      MOZ_ASSERT(gl->IsCurrent());

remove or preserve the "MOZ_ASSERT(gl->IsCurrent())" as DrawArrays_check().
Attachment #8792681 - Flags: review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review78408

::: dom/canvas/WebGLFramebuffer.cpp:389
(Diff revision 1)
>  
> -        case LOCAL_GL_TEXTURE_2D_ARRAY:
> +    case LOCAL_GL_TEXTURE_2D_ARRAY:
> -        case LOCAL_GL_TEXTURE_3D:
> +    case LOCAL_GL_TEXTURE_3D:
> -            if (attachment == LOCAL_GL_DEPTH_STENCIL_ATTACHMENT) {
> -                gl->fFramebufferTextureLayer(target,
> -                                             LOCAL_GL_DEPTH_ATTACHMENT, glName, mipLevel,
> +        // If we have fFramebufferTextureLayer, we can rely on having
> +        // DEPTH_STENCIL_ATTACHMENT.
> +        gl->fFramebufferTextureLayer(target.get(), mAttachmentPoint,

In webgl2 spec "4.1.5 Framebuffer Object Attachments":

In WebGL 1, DEPTH_STENCIL_ATTACHMENT is an alternative attachment point other than DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In WebGL 2, however, DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT, i.e., the same image is attached to both DEPTH_ATTACHMENT and STENCIL_ATTACHMENT, overwriting the original images attached to the two attachment points.

How about the usage for following sequence of actions when using FramebufferTextureLayer?
    attach renderbuffer 1 to DEPTH_ATTACHMENT;
    attach renderbuffer 2 to DEPTH_STENCIL_ATTACHMENT;
    attach null to DEPTH_STENCIL_ATTACHMENT.

Comment 14

2 years ago
mozreview-review
Comment on attachment 8792682 [details]
Bug 1303879 - FramebufferTexture2D doesn't always accept DEPTH_STENCIL. -

https://reviewboard.mozilla.org/r/79592/#review78410
Attachment #8792682 - Flags: review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8792683 [details]
Bug 1303879 - fb->ReadBuffer(BACK) is INVALID_OP. -

https://reviewboard.mozilla.org/r/79594/#review78434
Attachment #8792683 - Flags: review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8792683 [details]
Bug 1303879 - fb->ReadBuffer(BACK) is INVALID_OP. -

https://reviewboard.mozilla.org/r/79594/#review78436
Attachment #8792683 - Flags: review+

Comment 17

2 years ago
mozreview-review
Comment on attachment 8792684 [details]
Bug 1303879 - Disallow NONE GetFramebufferAttachmentParameter. -

https://reviewboard.mozilla.org/r/79596/#review78440

::: dom/canvas/WebGLFramebuffer.cpp:1473
(Diff revision 1)
>  WebGLFramebuffer::GetAttachmentParameter(const char* funcName, JSContext* cx,
>                                           GLenum target, GLenum attachEnum, GLenum pname,
>                                           ErrorResult* const out_error)
>  {
>      const auto maybeAttach = GetAttachPoint(attachEnum);
> -    if (!maybeAttach) {
> +    if (!maybeAttach || attachEnum == LOCAL_GL_NONE) {

I think GetAttachPoint(GL_NONE) will return nullptr.
So, this checking is redundant.

https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/dom/canvas/WebGLFramebuffer.cpp#794

Comment 18

2 years ago
mozreview-review
Comment on attachment 8792681 [details]
Bug 1303879 - Validate attachments inside ScopedResolveTextures. -

https://reviewboard.mozilla.org/r/79590/#review78442
Attachment #8792681 - Flags: review+

Comment 19

2 years ago
mozreview-review
Comment on attachment 8792686 [details]
Bug 1303879 - Explicitly nuke attachment points. -

https://reviewboard.mozilla.org/r/79600/#review78446
Attachment #8792686 - Flags: review+

Comment 20

2 years ago
mozreview-review
Comment on attachment 8792686 [details]
Bug 1303879 - Explicitly nuke attachment points. -

https://reviewboard.mozilla.org/r/79600/#review78450
Attachment #8792686 - Flags: review+

Comment 21

2 years ago
mozreview-review
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78452

::: dom/canvas/WebGLFramebuffer.cpp:1021
(Diff revision 1)
>              gl::ScopedBindFramebuffer autoBind(mContext->gl, mGLName);
>  
>              mContext->ForceClearFramebufferWithDefaultValues(clearBits, false);
>          }
>  
> -        fnDrawBuffers(mColorDrawBuffers);
> +        if (mContext->HasDrawBuffers()) {

You should use 'hasDrawBuffers' here.

::: dom/canvas/WebGLFramebuffer.cpp:1170
(Diff revision 1)
>  ////
>  
>  void
> +WebGLFramebuffer::RefreshDrawBuffers() const
> +{
> +	const auto& gl = mContext->gl;

The intentation is wrong.

Comment 22

2 years ago
mozreview-review
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78460

::: dom/canvas/WebGLFramebuffer.cpp:1192
(Diff revision 1)
> +}
> +
> +void
> +WebGLFramebuffer::RefreshReadBuffer() const
> +{
> +	const auto& gl = mContext->gl;

Please fix the indentation too.

Comment 23

2 years ago
mozreview-review
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78454

::: dom/canvas/WebGLContext.cpp:2131
(Diff revision 1)
> +{
> +    const auto fnName = [&](WebGLFramebuffer* fb) {
> +        return fb ? fb->mGLName : 0;
> +    };
> +
> +    if (mWebGL->IsWebGL2()) {

webgl2 note:
https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.4.pdf p201

Calling BindFramebuffer with target set to FRAMEBUFFER binds framebuffer to both the draw and read targets

::: dom/canvas/WebGLFramebuffer.cpp:1269
(Diff revision 1)
>          } else {
>              mContext->ErrorInvalidEnum(text, funcName);
>          }
>          return;
>      }
> -    const auto& attach = maybeAttach.value();
> +    const auto& attach = maybeAttach.value(); // Might be nullptr.

If maybeAttach.value() is nullptr, it will has an early return from the previous if block. So, this comment is not correct.

Comment 24

2 years ago
mozreview-review
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78548

::: dom/canvas/WebGLFramebuffer.cpp:1186
(Diff revision 1)
> +            const uint32_t index = attach->mAttachmentPoint - LOCAL_GL_COLOR_ATTACHMENT0;
> +            driverBuffers[index] = attach->mAttachmentPoint;
> +        }
> +    }
> +
> +    gl->fDrawBuffers(driverBuffers.size(), driverBuffers.data());

GL4.0 has FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER checking, but GL4.1 and upon versions don't.

This function try to fill GL_NONE for all no-image attachments to prevent the FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER error. Thus, there is no FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER error anymore.
Is my description correct?

::: dom/canvas/WebGLFramebuffer.cpp:1205
(Diff revision 1)
> +    GLenum driverBuffer = LOCAL_GL_NONE;
> +    if (mColorReadBuffer && mColorReadBuffer->HasImage()) {
> +        driverBuffer = mColorReadBuffer->mAttachmentPoint;
> +    }
> +
> +    gl->fReadBuffer(driverBuffer);

Similar to the comment in RefreshDrawBuffer.
(Assignee)

Comment 25

2 years ago
mozreview-review-reply
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review78408

> In webgl2 spec "4.1.5 Framebuffer Object Attachments":
> 
> In WebGL 1, DEPTH_STENCIL_ATTACHMENT is an alternative attachment point other than DEPTH_ATTACHMENT and STENCIL_ATTACHMENT. In WebGL 2, however, DEPTH_STENCIL_ATTACHMENT is considered an alias for DEPTH_ATTACHMENT + STENCIL_ATTACHMENT, i.e., the same image is attached to both DEPTH_ATTACHMENT and STENCIL_ATTACHMENT, overwriting the original images attached to the two attachment points.
> 
> How about the usage for following sequence of actions when using FramebufferTextureLayer?
>     attach renderbuffer 1 to DEPTH_ATTACHMENT;
>     attach renderbuffer 2 to DEPTH_STENCIL_ATTACHMENT;
>     attach null to DEPTH_STENCIL_ATTACHMENT.

We handle this here and elsewhere.
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
Comment on attachment 8792684 [details]
Bug 1303879 - Disallow NONE GetFramebufferAttachmentParameter. -

https://reviewboard.mozilla.org/r/79596/#review78440

> I think GetAttachPoint(GL_NONE) will return nullptr.
> So, this checking is redundant.
> 
> https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/dom/canvas/WebGLFramebuffer.cpp#794

It returns Some(nullptr), so we need to check it separately. (having this behavior is useful elsewhere. We might change it later, though)
(Assignee)

Comment 27

2 years ago
mozreview-review-reply
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78548

> GL4.0 has FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER checking, but GL4.1 and upon versions don't.
> 
> This function try to fill GL_NONE for all no-image attachments to prevent the FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER error. Thus, there is no FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER error anymore.
> Is my description correct?

Yes.

> Similar to the comment in RefreshDrawBuffer.

Yes.
(Assignee)

Comment 28

2 years ago
mozreview-review-reply
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78454

> webgl2 note:
> https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.4.pdf p201
> 
> Calling BindFramebuffer with target set to FRAMEBUFFER binds framebuffer to both the draw and read targets

Yes, this is handled. What's the problem?

> If maybeAttach.value() is nullptr, it will has an early return from the previous if block. So, this comment is not correct.

No, because GetColorAttachPoint returns Some(nullptr), which is truthy. bool(Some(nullptr)) is true, bool(Some(nullptr).value) is false.

Comment 29

2 years ago
mozreview-review
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78630

::: dom/canvas/WebGLFramebuffer.cpp:1276
(Diff revision 1)
> -    RecacheResolvedData();
>  
>      mContext->MakeContextCurrent();
> -    mContext->gl->fReadBuffer(attachPoint);
> +
> +    mColorReadBuffer = attach;
> +    RefreshDrawBuffers(); // Calls glReadBuffer.

Here should be RefreshReadBuffers();

Comment 30

2 years ago
mozreview-review
Comment on attachment 8792684 [details]
Bug 1303879 - Disallow NONE GetFramebufferAttachmentParameter. -

https://reviewboard.mozilla.org/r/79596/#review78632
Attachment #8792684 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8792689 - Attachment is obsolete: true

Comment 39

2 years ago
mozreview-review
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78772

::: dom/canvas/WebGLFramebuffer.cpp:1136
(Diff revision 2)
> +
>          ////
>  
> -        const FBTarget fbTarget = (mContext->IsWebGL2() ? LOCAL_GL_DRAW_FRAMEBUFFER
> -                                                        : LOCAL_GL_FRAMEBUFFER);
> -        const bool needsFBRebind = (mContext->mBoundDrawFramebuffer != this);
> +        ResolveAttachments(); // OK, attach everything properly!
> +        RefreshDrawBuffers();
> +        RefreshReadBuffer();

Why we need to call RefreshDraw/ReadBuffer() here?

I'm not sure the drawBuffer and readBuffer status should belong to the WebGLFramebuffer object.

If we do something like:
1. create FBO1
2. bind FBO1
3. attach color_attachment_0, color_attachment_1 to FBO1
4. call DrawBuffers() to set the fragment shader's output 0 and 1 to color_attachment_0 and color_attachment_1
5. create FBO2
6. bind FBO2

I think the DrawBuffers() status doesn't change even though we bind to FBO2.
Maybe we should save the draw/readBuffer status in webgl context. And handle them here properly to avoid the "FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER" error for framebuffer status checking.

Maybe I am wrong. Could you point to the spec. to show up the draw/readBuffer status should save in FBO or in context?

::: dom/canvas/WebGLFramebuffer.cpp
(Diff revision 2)
>          ////
>  
> -        const FBTarget fbTarget = (mContext->IsWebGL2() ? LOCAL_GL_DRAW_FRAMEBUFFER
> -                                                        : LOCAL_GL_FRAMEBUFFER);
> -        const bool needsFBRebind = (mContext->mBoundDrawFramebuffer != this);
> +        ResolveAttachments(); // OK, attach everything properly!
> +        RefreshDrawBuffers();
> +        RefreshReadBuffer();
> -        if (needsFBRebind) {

Why do we remove the optimization for binding the same fbo here?
(Assignee)

Comment 40

2 years ago
mozreview-review-reply
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78772

> Why we need to call RefreshDraw/ReadBuffer() here?
> 
> I'm not sure the drawBuffer and readBuffer status should belong to the WebGLFramebuffer object.
> 
> If we do something like:
> 1. create FBO1
> 2. bind FBO1
> 3. attach color_attachment_0, color_attachment_1 to FBO1
> 4. call DrawBuffers() to set the fragment shader's output 0 and 1 to color_attachment_0 and color_attachment_1
> 5. create FBO2
> 6. bind FBO2
> 
> I think the DrawBuffers() status doesn't change even though we bind to FBO2.
> Maybe we should save the draw/readBuffer status in webgl context. And handle them here properly to avoid the "FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER" error for framebuffer status checking.
> 
> Maybe I am wrong. Could you point to the spec. to show up the draw/readBuffer status should save in FBO or in context?

You are incorrect. This state is attached to the FBO. (GLES 3.0.4 p256)

> Why do we remove the optimization for binding the same fbo here?

It added complexity for marginal benefit. It's easier to just get it right. This code runs very rarely, since we cache FRAMEBUFFER_COMPLETE status.

Comment 41

2 years ago
mozreview-review
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78986
Attachment #8792687 - Flags: review+

Comment 42

2 years ago
mozreview-review
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review78990
Attachment #8792687 - Flags: review+
Comment on attachment 8792681 [details]
Bug 1303879 - Validate attachments inside ScopedResolveTextures. -

https://reviewboard.mozilla.org/r/79590/#review78998
Attachment #8792681 - Flags: review?(mtseng) → review+
Comment on attachment 8792682 [details]
Bug 1303879 - FramebufferTexture2D doesn't always accept DEPTH_STENCIL. -

https://reviewboard.mozilla.org/r/79592/#review79000
Attachment #8792682 - Flags: review?(mtseng) → review+
Comment on attachment 8792683 [details]
Bug 1303879 - fb->ReadBuffer(BACK) is INVALID_OP. -

https://reviewboard.mozilla.org/r/79594/#review79004
Attachment #8792683 - Flags: review?(mtseng) → review+
Comment on attachment 8792684 [details]
Bug 1303879 - Disallow NONE GetFramebufferAttachmentParameter. -

https://reviewboard.mozilla.org/r/79596/#review79006
Attachment #8792684 - Flags: review?(mtseng) → review+
Comment on attachment 8792685 [details]
Bug 1303879 - Use explicit ctors. -

https://reviewboard.mozilla.org/r/79598/#review79008
Attachment #8792685 - Flags: review?(mtseng) → review+
Comment on attachment 8792686 [details]
Bug 1303879 - Explicitly nuke attachment points. -

https://reviewboard.mozilla.org/r/79600/#review79010
Attachment #8792686 - Flags: review?(mtseng) → review+
Comment on attachment 8792687 [details]
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -

https://reviewboard.mozilla.org/r/79602/#review79012
Attachment #8792687 - Flags: review?(mtseng) → review+

Comment 50

2 years ago
mozreview-review
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79018

::: dom/canvas/WebGLFramebuffer.cpp:1299
(Diff revision 1)
> +    // `texImageTarget`
> +    if (texImageTarget != LOCAL_GL_TEXTURE_2D &&
> +        (texImageTarget < LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X ||
> +         texImageTarget > LOCAL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Z))
> +    {
> +        mContext->ErrorInvalidEnumInfo("framebufferTexture2D: texImageTarget:",

I think here should set 'funcName' to ErrorInvalidEnumInfo.

Comment 51

2 years ago
mozreview-review
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79016

::: dom/canvas/WebGL2ContextFramebuffers.cpp:73
(Diff revision 1)
>  {
> +    const char funcName[] = "framebufferTextureLayer";
>      if (IsContextLost())
>          return;
>  
> -    if (!ValidateFramebufferTarget(target, "framebufferTextureLayer"))
> +    if (!ValidateFramebufferTarget(target, funcName))

This is already checked in WebGLFramebuffer::FramebufferTextureLayer(). Is it redundant?

::: dom/canvas/WebGL2ContextFramebuffers.cpp:96
(Diff revision 1)
> -        if (layer < 0)
> -            return ErrorInvalidValue("framebufferTextureLayer: layer must be >= 0.");
> +    if (!fb) {
> +        return ErrorInvalidOperation("framebufferTextureLayer: cannot modify"
> +                                     " framebuffer 0.");
> +    }
>  
> -        if (level < 0)
> +    if (!ValidateObjectAllowNull("framebufferTextureLayer: texture", texture))

This is also checked in WebGLFramebuffer::FramebufferTextureLayer().

::: dom/canvas/WebGL2ContextFramebuffers.cpp:99
(Diff revision 1)
> +    }
>  
> -        if (level < 0)
> -            return ErrorInvalidValue("framebufferTextureLayer: level must be >= 0.");
> +    if (!ValidateObjectAllowNull("framebufferTextureLayer: texture", texture))
> +        return;
>  
> +    if (texture) {

And this whole block is also checked in WebGLFramebuffer::FramebufferTextureLayer().
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79014

::: dom/canvas/WebGLFramebuffer.h:110
(Diff revision 1)
> -            cur.~T();
> -        }
> -        free(mArray);
> -    }
>  
> -    T* begin() const {
> +        Ordered(const WebGLFBAttachPoint& ref)

explicit constructor.

::: dom/canvas/WebGLFramebuffer.cpp:1039
(Diff revision 1)
> -        {
> +{
> +    if (parent.mDepthAttachment.IsDefined()) {
> +        depthBuffer = &parent.mDepthAttachment;
> +    }
> +    if (parent.mStencilAttachment.IsDefined()) {
> +        stencilBuffer = &parent.mDepthAttachment;

Should be mStencilAttachment.

::: dom/canvas/WebGLFramebuffer.cpp:1601
(Diff revision 1)
> +    // Clear unused buffer bits
> +
> +    if (mask & LOCAL_GL_COLOR_BUFFER_BIT &&
> +        !srcColorFormat && !dstHasColor)
> +    {
> +

Useless blank line.

::: dom/canvas/WebGLFramebuffer.cpp:1643
(Diff revision 1)
> +        }
> +    }
> +
> +    const GLbitfield depthAndStencilBits = LOCAL_GL_DEPTH_BUFFER_BIT |
> +                                           LOCAL_GL_STENCIL_BUFFER_BIT;
> +    if (bool(mask & depthAndStencilBits) &&

Why a cast here? Looks unnecessary.

Comment 53

2 years ago
mozreview-review-reply
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79014

> explicit constructor.

Looks like be fixed by later patch.
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79040
Attachment #8792680 - Flags: review?(mtseng) → review+
(Assignee)

Comment 55

2 years ago
mozreview-review-reply
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79016

> This is already checked in WebGLFramebuffer::FramebufferTextureLayer(). Is it redundant?

No it's not.
(Assignee)

Comment 56

2 years ago
mozreview-review-reply
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79018

> I think here should set 'funcName' to ErrorInvalidEnumInfo.

I would need to change the signature for that function, IIRC. (I could be wrong)
(Assignee)

Comment 57

2 years ago
mozreview-review-reply
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79014

> Looks like be fixed by later patch.

It is.
(Assignee)

Comment 58

2 years ago
mozreview-review-reply
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

https://reviewboard.mozilla.org/r/79588/#review79014

> Why a cast here? Looks unnecessary.

It makes it explicit that I meant `mask & depthAndStencilBits` not `mask && depthAndStencilBits`.
(Assignee)

Updated

2 years ago
Depends on: 1303878

Comment 59

2 years ago
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34578d0c9449
Refactor framebuffer funcs and completeness caching. - r=mtseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/43051f6dc9c1
Validate attachments inside ScopedResolveTextures. - r=mtseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/709b8e20ba72
FramebufferTexture2D doesn't always accept DEPTH_STENCIL. - r=mtseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/2208ecafeea8
fb->ReadBuffer(BACK) is INVALID_OP. - r=mtseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cc98e1be21c
Disallow NONE GetFramebufferAttachmentParameter. - r=mtseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df95decfa8c
Use explicit ctors. - r=mtseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0429ced124f
Explicitly nuke attachment points. - r=mtseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e71c7094eb1
Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. - r=mtseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/085c08ea65c8
WebGL1 draw-buffers ext test passes on OSX.
(Assignee)

Comment 62

2 years ago
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]: bad webgl2
[Describe test coverage new/current, TreeHerder]: conformance suite
[Risks and why]: low. This is pretty well understood.
[String/UUID change made/needed]: none

This applies to all patches in this bug.
Attachment #8792680 - Flags: approval-mozilla-aurora?
status-firefox50: --- → affected
status-firefox51: --- → affected
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

WebGL2 is planned for Beta50, let's stabilize this on Aurora51 and if there are no problems I plan to include this in 50.0b4 on 10/03.
Attachment #8792680 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

2 years ago
Blocks: 1306480
I hit conflicts trying to uplift this to beta just now.
Flags: needinfo?(jgilbert)
Comment on attachment 8792680 [details]
Bug 1303879 - Refactor framebuffer funcs and completeness caching. -

WebGL2 is planned for Fx50, Beta+ for all patches.
Attachment #8792680 - Flags: approval-mozilla-beta+
(Assignee)

Updated

2 years ago
Depends on: 1289655
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.