Fix BlitFramebuffer

RESOLVED FIXED in Firefox 50

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 1 obsolete attachment)

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

Description

a year 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

a year 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 12

a year ago
mozreview-review
Comment on attachment 8792685 [details]
Bug 1303879 - Use explicit ctors. -

https://reviewboard.mozilla.org/r/79598/#review78376
Attachment #8792685 - Flags: review+

Comment 13

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
Attachment #8792689 - Attachment is obsolete: true

Comment 39

a year 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

a year 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

a year 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

a year 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 43

a year ago
mozreview-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 44

a year ago
mozreview-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 45

a year ago
mozreview-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 46

a year ago
mozreview-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 47

a year ago
mozreview-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 48

a year ago
mozreview-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 49

a year 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/#review79012
Attachment #8792687 - Flags: review?(mtseng) → review+

Comment 50

a year 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

a year 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 52

a year ago
mozreview-review
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

a year 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 54

a year ago
mozreview-review
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

a year 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

a year 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

a year 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

a year 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

a year ago
Depends on: 1303878

Comment 59

a year 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.

Comment 60

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34578d0c9449
https://hg.mozilla.org/mozilla-central/rev/43051f6dc9c1
https://hg.mozilla.org/mozilla-central/rev/709b8e20ba72
https://hg.mozilla.org/mozilla-central/rev/2208ecafeea8
https://hg.mozilla.org/mozilla-central/rev/2cc98e1be21c
https://hg.mozilla.org/mozilla-central/rev/7df95decfa8c
https://hg.mozilla.org/mozilla-central/rev/c0429ced124f
https://hg.mozilla.org/mozilla-central/rev/5e71c7094eb1
https://hg.mozilla.org/mozilla-central/rev/085c08ea65c8
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 61

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34578d0c9449
https://hg.mozilla.org/mozilla-central/rev/43051f6dc9c1
https://hg.mozilla.org/mozilla-central/rev/709b8e20ba72
https://hg.mozilla.org/mozilla-central/rev/2208ecafeea8
https://hg.mozilla.org/mozilla-central/rev/2cc98e1be21c
https://hg.mozilla.org/mozilla-central/rev/7df95decfa8c
https://hg.mozilla.org/mozilla-central/rev/c0429ced124f
https://hg.mozilla.org/mozilla-central/rev/5e71c7094eb1
https://hg.mozilla.org/mozilla-central/rev/085c08ea65c8
(Assignee)

Comment 62

a year 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?
Blocks: 1286871

Updated

a year ago
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

a year ago
Blocks: 1306480

Comment 64

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec6e14759d10
https://hg.mozilla.org/releases/mozilla-aurora/rev/2b689ba2cc20
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6e11c04184f
https://hg.mozilla.org/releases/mozilla-aurora/rev/9bb95cc7e0d1
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1fef9fa9a73
https://hg.mozilla.org/releases/mozilla-aurora/rev/f211bc3bbdbe
https://hg.mozilla.org/releases/mozilla-aurora/rev/fd8d653ca590
https://hg.mozilla.org/releases/mozilla-aurora/rev/069e4074df40
https://hg.mozilla.org/releases/mozilla-aurora/rev/73efde7745ee
status-firefox51: affected → fixed
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+
Jeff landed this. Had to be backed out for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=1706115&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/ca2593daaa91f717e3ae65f049308309a31969a2
(Assignee)

Updated

a year ago
Depends on: 1289655
Flags: needinfo?(jgilbert)
(Assignee)

Comment 68

a year ago
https://hg.mozilla.org/releases/mozilla-beta/rev/66081ef243719281e97f8ffbf958edc1a5c59972
status-firefox50: affected → fixed
You need to log in before you can comment on or make changes to this bug.