Closed
Bug 1303879
Opened 8 years ago
Closed 8 years ago
Fix BlitFramebuffer
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
Attachments
(9 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
mtseng
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
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 |
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•8 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 12•8 years 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Attachment #8792689 -
Attachment is obsolete: true
Comment 39•8 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•8 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•8 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•8 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 43•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 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/#review79012
Attachment #8792687 -
Flags: review?(mtseng) → review+
Comment 50•8 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•8 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 52•8 years 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•8 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 54•8 years 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•8 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•8 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•8 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•8 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`.
Comment 59•8 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.
Comment 60•8 years 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
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 61•8 years 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•8 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+
Comment 64•8 years ago
|
||
bugherder uplift |
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
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+
Comment 67•8 years ago
|
||
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 | ||
Comment 68•8 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•