Closed Bug 1303879 Opened 8 years ago Closed 8 years ago

Fix BlitFramebuffer

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(9 files, 1 obsolete file)

Bug 1303879 - Refactor framebuffer funcs and completeness caching. -
58 bytes, text/x-review-board-request
mtseng
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details
Bug 1303879 - Validate attachments inside ScopedResolveTextures. -
58 bytes, text/x-review-board-request
mtseng
: review+
Details
Bug 1303879 - FramebufferTexture2D doesn't always accept DEPTH_STENCIL. -
58 bytes, text/x-review-board-request
mtseng
: review+
Details
Bug 1303879 - fb->ReadBuffer(BACK) is INVALID_OP. -
58 bytes, text/x-review-board-request
mtseng
: review+
Details
Bug 1303879 - Disallow NONE GetFramebufferAttachmentParameter. -
58 bytes, text/x-review-board-request
mtseng
: review+
Details
Bug 1303879 - Use explicit ctors. -
58 bytes, text/x-review-board-request
mtseng
: review+
Details
Bug 1303879 - Explicitly nuke attachment points. -
58 bytes, text/x-review-board-request
mtseng
: review+
Details
Bug 1303879 - Refresh DrawBuffers and ReadBuffer to handle GL4.0 and below. -
58 bytes, text/x-review-board-request
mtseng
: review+
ethlin
: review+
jerry
: review+
Details
WebGL1 draw-buffers ext test passes on OSX.
58 bytes, text/x-review-board-request
Details 
In particular, blitFramebuffer doesn't handle feedback right now.
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 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 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 on attachment 8792683 [details]
Bug 1303879 - fb->ReadBuffer(BACK) is INVALID_OP. -

https://reviewboard.mozilla.org/r/79594/#review78434
Attachment #8792683 - Flags: 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 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 on attachment 8792681 [details]
Bug 1303879 - Validate attachments inside ScopedResolveTextures. -

https://reviewboard.mozilla.org/r/79590/#review78442
Attachment #8792681 - Flags: review+
Comment on attachment 8792686 [details]
Bug 1303879 - Explicitly nuke attachment points. -

https://reviewboard.mozilla.org/r/79600/#review78446
Attachment #8792686 - Flags: review+
Comment on attachment 8792686 [details]
Bug 1303879 - Explicitly nuke attachment points. -

https://reviewboard.mozilla.org/r/79600/#review78450
Attachment #8792686 - Flags: 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 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 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 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.
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.
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)
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.
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 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 on attachment 8792684 [details]
Bug 1303879 - Disallow NONE GetFramebufferAttachmentParameter. -

https://reviewboard.mozilla.org/r/79596/#review78632
Attachment #8792684 - Flags: review+
Attachment #8792689 - Attachment is obsolete: true
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?
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 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 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 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 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 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+
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.
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)
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.
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`.
Depends on: 1303878
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 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?
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+
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+
Depends on: 1289655
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: