Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1852533002:
Remove alpha/depth/stencil/antialias from WGC3D::Attributes. (Closed)
|
Created:
4 years, 8 months ago by danakj Modified:
4 years, 8 months ago CC:
chrishtr, bajones, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, dcheng, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jam, jbroman, Justin Novosad, Ken Russell (switch to Gerrit), kinuko+watch, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, pdr+graphicswatchlist_chromium.org, piman+watch_chromium.org, piman, rwlbuis, Stephen Chennney, no sievers, Zhenyao Mo Base URL:
https://chromium.googlesource.com/chromium/src.git@premul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove alpha/depth/stencil/antialias from WGC3D::Attributes.
This removes the alpha, depth, stencil and antialias fields from the
WebGraphicsContext3D::Attributes structure. These fields control
properties of the default framebuffer. But since WebGL uses an
offscreen context, the default frame buffer is not used so these
attributes have no impact on the output of WebGL.
Instead, pass the desired values as independent bools to DrawingBuffer
and have it pass those around during setup (instead of storing them
as members).
R=chrishtr@chromium.org, kbr@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/7b9fe38d09d734b5a8ea838ed232f7dee0662d85
Cr-Commit-Position: refs/heads/master@{#384757}
Patch Set 1 #
Total comments: 16
Patch Set 2 : rm-alphadepthetc: . #Patch Set 3 : rm-alphadepthetc: rebase #
Total comments: 7
Patch Set 4 : rm-alphadepthetc: notruelies #
Total comments: 3
Patch Set 5 : rm-alphadepthetc: testexpect #
Total comments: 2
Patch Set 6 : rm-alphadepthetc: testgeez #
Total comments: 6
Patch Set 7 : rm-alphadepthetc: rebase #Patch Set 8 : rm-alphadepthetc: renamevar #Messages
Total messages: 51 (15 generated)
Description was changed from ========== Remove alpha/depth/stencil/antialias from WGC3D::Attributes. This removes the alpha, depth, stencil and antialias fields from the WebGraphicsContext3D::Attributes structure. These fields control properties of the default framebuffer. But since WebGL uses an offscreen context, the default frame buffer is not used so these attributes have no impact on the output of WebGL. Instead, pass the desired values as independent bools to DrawingBuffer and have it pass those around during setup (instead of storing them as members). R=chrishtr@chromium.org, kbr@chromium.org BUG=584497 ========== to ========== Remove alpha/depth/stencil/antialias from WGC3D::Attributes. This removes the alpha, depth, stencil and antialias fields from the WebGraphicsContext3D::Attributes structure. These fields control properties of the default framebuffer. But since WebGL uses an offscreen context, the default frame buffer is not used so these attributes have no impact on the output of WebGL. Instead, pass the desired values as independent bools to DrawingBuffer and have it pass those around during setup (instead of storing them as members). R=chrishtr@chromium.org, kbr@chromium.org BUG=584497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1852533002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1852533002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
danakj@chromium.org changed reviewers: + piman@chromium.org
kbr: everything piman: content chrishtr: public https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLContextAttributeHelpers.cpp (left): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLContextAttributeHelpers.cpp:32: if (settings && !settings->openGLMultisamplingEnabled()) This now leaves this setting unused (though it had no impact being here). I'll delete it in a followup. https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:887: bool wantAntialiasing = m_requestedAttributes.antialias(); WebGL can still turn off MSAA if the author requests, that was never broken. https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1309: bool wantAlphaChannel = m_requestedAttributes.alpha(); I assume that m_requestedAttributes is still valid here, please confirm. https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:690: if (!m_requestedAttributes.depth && !m_requestedAttributes.stencil) This is the early out we don't need then https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:722: return m_depthStencilBuffer && m_requestedAttributes.depth && !m_requestedAttributes.stencil; This is stored as a bool in initialize() now https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:110: wantAntialiasing This is where we disable MSAA if WebGL app said no. https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:295: frontColorBufferMailbox = createNewMailbox(createTextureAndAllocateMemory(m_size, m_hasAlphaChannel)); This is a possible behaviour change tho I'm not sure. The old behaviour would use the m_requestedAttributes to inform if we want RGB or RGBA. The new behaviour effectively uses the m_actualAttributes to inform if we want RGB or RGBA. IOW we will continue to get RGB or RGBA based on what happened in initialize(). But would that change? https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:496: m_hasImplicitStencilBuffer = true; This replaces the hasImplicitStencilBuffer() function, now it just returns this bool. It used to check that wantDepthBuffer was true, but we don't actually need to, we should not get m_depthStencilBuffer if it wasn't and wantStencilBuffer wasn't. So DCHECK instead. https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:674: if (m_antiAliasingMode != MSAAExplicitResolve && wantDepthOrStencilBuffer) we just don't call resizeDepthStencil() instead of passing this bool just to early out https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:694: if (wantDepthOrStencilBuffer) same here https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1027: bool wantAlphaChannel = false; // Not relevant for the target. It's a bit weird we construct the full texture params just for the target, but whatevs. We don't need to plumb desire for alpha here to get the target.
https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:500: m_gl->GetIntegerv(GL_ALPHA_BITS, &alphaBits); Oh, these seem to be wrong maybe now. They read about the default buffer not about the texture we created in reset()? Any suggestions?
https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:500: m_gl->GetIntegerv(GL_ALPHA_BITS, &alphaBits); On 2016/03/31 20:41:16, danakj wrote: > Oh, these seem to be wrong maybe now. They read about the default buffer not > about the texture we created in reset()? Any suggestions? For alpha.. we just use RGB or RGBA and we use that for texImage2D and stuff, should we just assume we have alpha on the texture if we want it? For depth/stencil, should we consider them true if m_depthStencilBuffer is non-zero?
lgtm https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:295: frontColorBufferMailbox = createNewMailbox(createTextureAndAllocateMemory(m_size, m_hasAlphaChannel)); On 2016/03/31 20:27:57, danakj wrote: > This is a possible behaviour change tho I'm not sure. > > The old behaviour would use the m_requestedAttributes to inform if we want RGB > or RGBA. > > The new behaviour effectively uses the m_actualAttributes to inform if we want > RGB or RGBA. > > IOW we will continue to get RGB or RGBA based on what happened in initialize(). > But would that change? Can we DCHECK they're the same? In particular, if we asked for RGB, if we were to get alpha bits, we definitely want to keep telling the compositor that it's opaque, so that we get culling, turn off blending, etc. I think today they're the same, but see TODO on l.975 where we want to emulate RGB on top of RGBA on some platforms that don't support RGB native buffers.
https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:500: m_gl->GetIntegerv(GL_ALPHA_BITS, &alphaBits); On 2016/03/31 20:46:43, danakj wrote: > On 2016/03/31 20:41:16, danakj wrote: > > Oh, these seem to be wrong maybe now. They read about the default buffer not > > about the texture we created in reset()? Any suggestions? > > For alpha.. we just use RGB or RGBA and we use that for texImage2D and stuff, > should we just assume we have alpha on the texture if we want it? > > For depth/stencil, should we consider them true if m_depthStencilBuffer is > non-zero? Hit send before seeing these questions. The values should be for the currently bound FBO, not the default framebuffer - so they give you the right things. But yes these queries seem not particularly useful (and costly - each one is a round trip) - we should know what we asked for. These made more sense when we used the default framebuffer, and we didn't know what we were getting.
PTAL https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:500: m_gl->GetIntegerv(GL_ALPHA_BITS, &alphaBits); On 2016/03/31 21:07:49, piman wrote: > On 2016/03/31 20:46:43, danakj wrote: > > On 2016/03/31 20:41:16, danakj wrote: > > > Oh, these seem to be wrong maybe now. They read about the default buffer not > > > about the texture we created in reset()? Any suggestions? > > > > For alpha.. we just use RGB or RGBA and we use that for texImage2D and stuff, > > should we just assume we have alpha on the texture if we want it? > > > > For depth/stencil, should we consider them true if m_depthStencilBuffer is > > non-zero? > > Hit send before seeing these questions. The values should be for the currently > bound FBO, not the default framebuffer - so they give you the right things. But > yes these queries seem not particularly useful (and costly - each one is a round > trip) - we should know what we asked for. These made more sense when we used the > default framebuffer, and we didn't know what we were getting. OK IIUC from all this and from IRC, then what I am going to do is the following: 0. Remove all these 3 queries. 1. Store m_wantAlphaChannel on DrawingBuffer, which says if we want A or not. This will not reflect the result of a query of GL, but rather just what texture format we're allocating. It can't change after construction, so I've made it const. The assumption then is that none of the code cares if what we got from GL doesn't match what we requested (or that this isn't possible). 2. Store m_hasDepthBuffer/m_hasStencilBuffer/m_hasImplicitStencilBuffer on DrawingBuffer. The first 2 are set to true IFF you requested them, and we allocated them. The latter is set to true IFF we didn't request it, and we allocated it. The assumption then is that if we Gen/Bind/FramebufferRenderbuffer for the STENCIL_DEPTH buffer, it would have a non-0 number of bits if we'd queried for them. 3. I stopped plumbing wantAlphaChannel all over now, since it's a member. Please check my assumptions.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1852533002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1852533002/40001
https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1309: bool wantDepthOrStencilBuffer = m_requestedAttributes.depth() || m_requestedAttributes.stencil(); kbr: Please check that reading m_requestAttributes here is right, and we don't need to store something else at the time of drawing buffer creation. https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:489: if (m_depthStencilBuffer) { Explanation of preserving old behaviour: If requested was false, we didn't branch before at all, so actual would be false for any one of these properties. That includes depth even tho stencil was true and we actually have depth bits, and vice versa. So here, we continue to set hasDepth and hasStencil to false if the request was false. hasImplicitStencilBuffer was implemented as m_depthStencilBuffer && m_requestedAttributes.depth && !m_requestedAttributes.stencil Now we check m_depthStencilBuffer still. If we have a buffer than wantDepth or wantStencil was true (and we DCHECK that). If wantStencil is false, then that implies that wantDepth was true, so we don't both to actually check that in the code. https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:774: if (m_hasDepthBuffer) { I will point out while I'm here that it's a bit weird we're branching on *has* depth/stencil here (or on the actual attributes before), since those are not set when this is called from initialize() until after this method returns. https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (left): https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:634: case GL_DEPTH_BITS: No longer queried
lgtm
One thing, outside of that it seems fine. https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:491: m_hasDepthBuffer = wantDepthBuffer; I'm not sure hasDepthBuffer/hasStencilBuffer are that useful if they're lies. The only non-test user (WebGLRenderingContextBase) already &&'s the result with the requested values, so I think we could just tell the truth?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:491: m_hasDepthBuffer = wantDepthBuffer; On 2016/04/01 00:24:15, piman wrote: > I'm not sure hasDepthBuffer/hasStencilBuffer are that useful if they're lies. > The only non-test user (WebGLRenderingContextBase) already &&'s the result with > the requested values, so I think we could just tell the truth? That's an excellent point. I had to think this through to confirm: Depth lies if - depth buffer exists - requested depth buffer was false In that case WebGLRCBase will not query hasDepthBuffer() at all: if (m_requestedAttributes.depth() && !drawingBuffer()->hasDepthBuffer()) So the fact that it lies doesn't matter. If the request is true, it will query. But if the request is true, we never lie. We report true if it exists and false otherwise. So ya I agree. There is, however, one other user of these lies, which is line 774ish in this file. Except there they read different lies, as I've commented there. They will be false always when called from initialize() so we won't clear. But when reset() is called externally, we won't clear the thing we didn't request. If we don't maintain these lies internally, we will clear the depth buffer when one wasn't requested, or stencil. WDYT?
https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:491: m_hasDepthBuffer = wantDepthBuffer; On 2016/04/01 00:40:50, danakj wrote: > On 2016/04/01 00:24:15, piman wrote: > > I'm not sure hasDepthBuffer/hasStencilBuffer are that useful if they're lies. > > The only non-test user (WebGLRenderingContextBase) already &&'s the result > with > > the requested values, so I think we could just tell the truth? > > That's an excellent point. I had to think this through to confirm: > > Depth lies if > - depth buffer exists > - requested depth buffer was false > > In that case WebGLRCBase will not query hasDepthBuffer() at all: > if (m_requestedAttributes.depth() && !drawingBuffer()->hasDepthBuffer()) > > So the fact that it lies doesn't matter. > > If the request is true, it will query. But if the request is true, we never lie. > We report true if it exists and false otherwise. > > So ya I agree. > > There is, however, one other user of these lies, which is line 774ish in this > file. Except there they read different lies, as I've commented there. > > They will be false always when called from initialize() so we won't clear. Oh, and if we use !!m_depthStencilBuffer there, then we'd clear on initialize() too. > But when reset() is called externally, we won't clear the thing we didn't > request. If we don't maintain these lies internally, we will clear the depth > buffer when one wasn't requested, or stencil. > > WDYT?
On Thu, Mar 31, 2016 at 5:40 PM, <danakj@chromium.org> wrote: > > > https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp > (right): > > > https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:491: > m_hasDepthBuffer = wantDepthBuffer; > On 2016/04/01 00:24:15, piman wrote: > > I'm not sure hasDepthBuffer/hasStencilBuffer are that useful if > they're lies. > > The only non-test user (WebGLRenderingContextBase) already &&'s the > result with > > the requested values, so I think we could just tell the truth? > > That's an excellent point. I had to think this through to confirm: > > Depth lies if > - depth buffer exists > - requested depth buffer was false > > In that case WebGLRCBase will not query hasDepthBuffer() at all: > if (m_requestedAttributes.depth() && > !drawingBuffer()->hasDepthBuffer()) > > So the fact that it lies doesn't matter. > > If the request is true, it will query. But if the request is true, we > never lie. We report true if it exists and false otherwise. > > So ya I agree. > > There is, however, one other user of these lies, which is line 774ish in > this file. Except there they read different lies, as I've commented > there. > > They will be false always when called from initialize() so we won't > clear. > > But when reset() is called externally, we won't clear the thing we > didn't request. If we don't maintain these lies internally, we will > clear the depth buffer when one wasn't requested, or stencil. > > WDYT? > I think we should always clear the buffers we requested, even if we don't want it. I mean, if we don't, the service side will anyway. > > https://codereview.chromium.org/1852533002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Mar 31, 2016 at 5:40 PM, <danakj@chromium.org> wrote: > > > https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp > (right): > > > https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:491: > m_hasDepthBuffer = wantDepthBuffer; > On 2016/04/01 00:24:15, piman wrote: > > I'm not sure hasDepthBuffer/hasStencilBuffer are that useful if > they're lies. > > The only non-test user (WebGLRenderingContextBase) already &&'s the > result with > > the requested values, so I think we could just tell the truth? > > That's an excellent point. I had to think this through to confirm: > > Depth lies if > - depth buffer exists > - requested depth buffer was false > > In that case WebGLRCBase will not query hasDepthBuffer() at all: > if (m_requestedAttributes.depth() && > !drawingBuffer()->hasDepthBuffer()) > > So the fact that it lies doesn't matter. > > If the request is true, it will query. But if the request is true, we > never lie. We report true if it exists and false otherwise. > > So ya I agree. > > There is, however, one other user of these lies, which is line 774ish in > this file. Except there they read different lies, as I've commented > there. > > They will be false always when called from initialize() so we won't > clear. > > But when reset() is called externally, we won't clear the thing we > didn't request. If we don't maintain these lies internally, we will > clear the depth buffer when one wasn't requested, or stencil. > > WDYT? > I think we should always clear the buffers we requested, even if we don't want it. I mean, if we don't, the service side will anyway. > > https://codereview.chromium.org/1852533002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Mar 31, 2016 at 5:51 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Thu, Mar 31, 2016 at 5:40 PM, <danakj@chromium.org> wrote: > >> >> >> https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... >> File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp >> (right): >> >> >> https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... >> third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:491: >> m_hasDepthBuffer = wantDepthBuffer; >> On 2016/04/01 00:24:15, piman wrote: >> > I'm not sure hasDepthBuffer/hasStencilBuffer are that useful if >> they're lies. >> > The only non-test user (WebGLRenderingContextBase) already &&'s the >> result with >> > the requested values, so I think we could just tell the truth? >> >> That's an excellent point. I had to think this through to confirm: >> >> Depth lies if >> - depth buffer exists >> - requested depth buffer was false >> >> In that case WebGLRCBase will not query hasDepthBuffer() at all: >> if (m_requestedAttributes.depth() && >> !drawingBuffer()->hasDepthBuffer()) >> >> So the fact that it lies doesn't matter. >> >> If the request is true, it will query. But if the request is true, we >> never lie. We report true if it exists and false otherwise. >> >> So ya I agree. >> >> There is, however, one other user of these lies, which is line 774ish in >> this file. Except there they read different lies, as I've commented >> there. >> >> They will be false always when called from initialize() so we won't >> clear. >> >> But when reset() is called externally, we won't clear the thing we >> didn't request. If we don't maintain these lies internally, we will >> clear the depth buffer when one wasn't requested, or stencil. >> >> WDYT? >> > > I think we should always clear the buffers we requested, even if we don't > want it. I mean, if we don't, the service side will anyway. > kk, done! > >> https://codereview.chromium.org/1852533002/ >> > > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Mar 31, 2016 at 5:51 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Thu, Mar 31, 2016 at 5:40 PM, <danakj@chromium.org> wrote: > >> >> >> https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... >> File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp >> (right): >> >> >> https://codereview.chromium.org/1852533002/diff/40001/third_party/WebKit/Sour... >> third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:491: >> m_hasDepthBuffer = wantDepthBuffer; >> On 2016/04/01 00:24:15, piman wrote: >> > I'm not sure hasDepthBuffer/hasStencilBuffer are that useful if >> they're lies. >> > The only non-test user (WebGLRenderingContextBase) already &&'s the >> result with >> > the requested values, so I think we could just tell the truth? >> >> That's an excellent point. I had to think this through to confirm: >> >> Depth lies if >> - depth buffer exists >> - requested depth buffer was false >> >> In that case WebGLRCBase will not query hasDepthBuffer() at all: >> if (m_requestedAttributes.depth() && >> !drawingBuffer()->hasDepthBuffer()) >> >> So the fact that it lies doesn't matter. >> >> If the request is true, it will query. But if the request is true, we >> never lie. We report true if it exists and false otherwise. >> >> So ya I agree. >> >> There is, however, one other user of these lies, which is line 774ish in >> this file. Except there they read different lies, as I've commented >> there. >> >> They will be false always when called from initialize() so we won't >> clear. >> >> But when reset() is called externally, we won't clear the thing we >> didn't request. If we don't maintain these lies internally, we will >> clear the depth buffer when one wasn't requested, or stencil. >> >> WDYT? >> > > I think we should always clear the buffers we requested, even if we don't > want it. I mean, if we don't, the service side will anyway. > kk, done! > >> https://codereview.chromium.org/1852533002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1852533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/1852533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:711: DepthStencilTestCase(true, true, 1, "depth only"), Are we testing 3 times the same thing? Should we fix the expectation instead?
https://codereview.chromium.org/1852533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/1852533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:711: DepthStencilTestCase(true, true, 1, "depth only"), On 2016/04/01 01:40:28, piman wrote: > Are we testing 3 times the same thing? Should we fix the expectation instead? Err.. haha oops. I read this as the expectations but you're right they are both.
https://codereview.chromium.org/1852533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/1852533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:711: DepthStencilTestCase(true, true, 1, "depth only"), On 2016/04/01 02:08:08, danakj wrote: > On 2016/04/01 01:40:28, piman wrote: > > Are we testing 3 times the same thing? Should we fix the expectation instead? > > Err.. haha oops. I read this as the expectations but you're right they are both. Done.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1852533002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1852533002/80001
LGTM after last fix https://codereview.chromium.org/1852533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/1852533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:710: DepthStencilTestCase(true, true, 1, "depth only"), false, true
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1852533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/1852533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:710: DepthStencilTestCase(true, true, 1, "depth only"), On 2016/04/01 02:55:25, piman wrote: > false, true Gah, sorry. Thanks.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1852533002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1852533002/100001
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1852533002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1852533002/120001
LGTM. Thanks for working on this; this is a quite tricky cleanup. https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:497: } Not necessary to revive this code -- but was it not necessary to test whether the GL actually provided what was asked for? It's fine to get rid of it. https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:458: bool DrawingBuffer::initialize(const IntSize& size, bool wantDepthBuffer, bool wantStencilBuffer, bool multisampleExtensionSupported) It would be worth documenting that "multisampleExtensionSupported" being true also now implies that antialiasing was requested. https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (left): https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:639: return; Why don't these need to be mocked any more? Does this change affect DrawingBufferDepthStencilTest::packedDepthStencilSupported?
https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:497: } On 2016/04/01 20:31:27, Ken Russell wrote: > Not necessary to revive this code -- but was it not necessary to test whether > the GL actually provided what was asked for? It's fine to get rid of it. GL doesn't have a say :) If it provides an extension, or if it's specified by the core, it must honor the request.
https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:458: bool DrawingBuffer::initialize(const IntSize& size, bool wantDepthBuffer, bool wantStencilBuffer, bool multisampleExtensionSupported) On 2016/04/01 20:31:27, Ken Russell wrote: > It would be worth documenting that "multisampleExtensionSupported" being true > also now implies that antialiasing was requested. Maybe I can rename this variable to useMultisample, kinda covers both. https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (left): https://codereview.chromium.org/1852533002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:639: return; On 2016/04/01 20:31:27, Ken Russell wrote: > Why don't these need to be mocked any more? Does this change affect > DrawingBufferDepthStencilTest::packedDepthStencilSupported? DrawingBuffer doesn't call this anymore. It just reports if it made m_deptStencilBuffer or not. Previously it only checked non-zero bits, and if it makes the buffer, then it has those bits.
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, piman@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/1852533002/#ps140001 (title: "rm-alphadepthetc: renamevar")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1852533002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1852533002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1852533002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1852533002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Remove alpha/depth/stencil/antialias from WGC3D::Attributes. This removes the alpha, depth, stencil and antialias fields from the WebGraphicsContext3D::Attributes structure. These fields control properties of the default framebuffer. But since WebGL uses an offscreen context, the default frame buffer is not used so these attributes have no impact on the output of WebGL. Instead, pass the desired values as independent bools to DrawingBuffer and have it pass those around during setup (instead of storing them as members). R=chrishtr@chromium.org, kbr@chromium.org BUG=584497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Remove alpha/depth/stencil/antialias from WGC3D::Attributes. This removes the alpha, depth, stencil and antialias fields from the WebGraphicsContext3D::Attributes structure. These fields control properties of the default framebuffer. But since WebGL uses an offscreen context, the default frame buffer is not used so these attributes have no impact on the output of WebGL. Instead, pass the desired values as independent bools to DrawingBuffer and have it pass those around during setup (instead of storing them as members). R=chrishtr@chromium.org, kbr@chromium.org BUG=584497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/7b9fe38d09d734b5a8ea838ed232f7dee0662d85 Cr-Commit-Position: refs/heads/master@{#384757} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7b9fe38d09d734b5a8ea838ed232f7dee0662d85 Cr-Commit-Position: refs/heads/master@{#384757} |
