Fix crash in BlitFramebufferCHROMIUM if a read or draw depth/stencil buffer is not present

Fix crash in BlitFramebufferCHROMIUM if a read or draw depth/stencil buffer is not present

From ES3 spec: "If a buffer is specified in mask and does not exist in both the read and draw
framebuffers, the corresponding bit is silently ignored."

BUG=648133
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/72e9876bdadd8cb4df5e813d03b5ca8d966d214f
Cr-Commit-Position: refs/heads/master@{#420207}

[piman, 2016-09-20 00:51:37]

Description was changed from

==========
Fix crash in BlitFramebufferCHROMIUM if a read or draw depth/stencil buffer is
not present

From ES3 spec: "If a buffer is specified in mask and does not exist in both the
read and draw
framebuffers, the corresponding bit is silently ignored."

BUG=648133
==========

to

==========
Fix crash in BlitFramebufferCHROMIUM if a read or draw depth/stencil buffer is
not present

From ES3 spec: "If a buffer is specified in mask and does not exist in both the
read and draw
framebuffers, the corresponding bit is silently ignored."

BUG=648133
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-09-20 00:52:11]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 00:52:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-09-20 00:52:45]

piman@chromium.org changed reviewers:
+ kbr@chromium.org

[commit-bot: I haz the power, 2016-09-20 01:27:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 01:27:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[piman, 2016-09-20 01:31:38]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 01:32:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 02:33:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 02:33:14]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Ken Russell (switch to Gerrit), 2016-09-20 23:24:24]

Sorry for the delay reviewing. The test failures look real and I can reproduce
them locally. Investigating them now.

[Ken Russell (switch to Gerrit), 2016-09-20 23:51:16]

On 2016/09/20 23:24:24, Ken Russell wrote:
> Sorry for the delay reviewing. The test failures look real and I can reproduce
> them locally. Investigating them now.

The failures are happening because WebGL 1.0 introduced a synthetic
GL_DEPTH_STENCIL_ATTACHMENT point distinct from GL_DEPTH_ATTACHMENT and
GL_STENCIL_ATTACHMENT. This was intended to allow portable support of
depth/stencil renderbuffers on systems that either supported separate
depth/stencil or packed depth/stencil. Now that Chrome only supports WebGL 1.0
on systems with packed depth/stencil we should simplify the code and remove the
command buffer's notion of the distinct GL_DEPTH_STENCIL_ATTACHMENT point.

The following added patch will make these tests pass:

diff --git a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
index a52ced0..24a8071 100644
--- a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
+++ b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
@@ -806,9 +806,8 @@ void DrawingBuffer::resizeDepthStencil(const IntSize& size)
         m_gl->RenderbufferStorageMultisampleCHROMIUM(GL_RENDERBUFFER,
m_sampleCount, GL_DEPTH24_STENCIL8_OES, size.width(), size.height());
     else
         m_gl->RenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH24_STENCIL8_OES,
size.width(), size.height());
-    // For ES 2.0 contexts DEPTH_STENCIL is not available natively, so we
emulate it
-    // at the command buffer level for WebGL contexts.
-    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT,
GL_RENDERBUFFER, m_depthStencilBuffer);
+    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT,
GL_RENDERBUFFER, m_depthStencilBuffer);
+    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT,
GL_RENDERBUFFER, m_depthStencilBuffer);
     m_gl->BindRenderbuffer(GL_RENDERBUFFER, 0);
 }

There's more cleanup work to be done though. How about pulling that patch into
yours and let's file a follow-up bug about cleaning things up in this area?

[Ken Russell (switch to Gerrit), 2016-09-20 23:52:16]

On 2016/09/20 23:51:16, Ken Russell wrote:
> On 2016/09/20 23:24:24, Ken Russell wrote:
> > Sorry for the delay reviewing. The test failures look real and I can
reproduce
> > them locally. Investigating them now.
> 
> The failures are happening because WebGL 1.0 introduced a synthetic
> GL_DEPTH_STENCIL_ATTACHMENT point distinct from GL_DEPTH_ATTACHMENT and
> GL_STENCIL_ATTACHMENT. This was intended to allow portable support of
> depth/stencil renderbuffers on systems that either supported separate
> depth/stencil or packed depth/stencil. Now that Chrome only supports WebGL 1.0
> on systems with packed depth/stencil we should simplify the code and remove
the
> command buffer's notion of the distinct GL_DEPTH_STENCIL_ATTACHMENT point.
> 
> The following added patch will make these tests pass:
> 
> diff --git a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> index a52ced0..24a8071 100644
> --- a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> +++ b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> @@ -806,9 +806,8 @@ void DrawingBuffer::resizeDepthStencil(const IntSize&
size)
>          m_gl->RenderbufferStorageMultisampleCHROMIUM(GL_RENDERBUFFER,
> m_sampleCount, GL_DEPTH24_STENCIL8_OES, size.width(), size.height());
>      else
>          m_gl->RenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH24_STENCIL8_OES,
> size.width(), size.height());
> -    // For ES 2.0 contexts DEPTH_STENCIL is not available natively, so we
> emulate it
> -    // at the command buffer level for WebGL contexts.
> -    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER,
GL_DEPTH_STENCIL_ATTACHMENT,
> GL_RENDERBUFFER, m_depthStencilBuffer);
> +    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT,
> GL_RENDERBUFFER, m_depthStencilBuffer);
> +    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT,
> GL_RENDERBUFFER, m_depthStencilBuffer);
>      m_gl->BindRenderbuffer(GL_RENDERBUFFER, 0);
>  }
> 
> There's more cleanup work to be done though. How about pulling that patch into
> yours and let's file a follow-up bug about cleaning things up in this area?

Sorry, that didn't format well. See here:
https://gist.github.com/kenrussell/2306bd42c3d677c0f6f49fe2b31308b0

[piman, 2016-09-21 00:21:08]

On 2016/09/20 23:51:16, Ken Russell wrote:
> On 2016/09/20 23:24:24, Ken Russell wrote:
> > Sorry for the delay reviewing. The test failures look real and I can
reproduce
> > them locally. Investigating them now.
> 
> The failures are happening because WebGL 1.0 introduced a synthetic
> GL_DEPTH_STENCIL_ATTACHMENT point distinct from GL_DEPTH_ATTACHMENT and
> GL_STENCIL_ATTACHMENT. This was intended to allow portable support of
> depth/stencil renderbuffers on systems that either supported separate
> depth/stencil or packed depth/stencil. Now that Chrome only supports WebGL 1.0
> on systems with packed depth/stencil we should simplify the code and remove
the
> command buffer's notion of the distinct GL_DEPTH_STENCIL_ATTACHMENT point.
> 
> The following added patch will make these tests pass:
> 
> diff --git a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> index a52ced0..24a8071 100644
> --- a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> +++ b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> @@ -806,9 +806,8 @@ void DrawingBuffer::resizeDepthStencil(const IntSize&
size)
>          m_gl->RenderbufferStorageMultisampleCHROMIUM(GL_RENDERBUFFER,
> m_sampleCount, GL_DEPTH24_STENCIL8_OES, size.width(), size.height());
>      else
>          m_gl->RenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH24_STENCIL8_OES,
> size.width(), size.height());
> -    // For ES 2.0 contexts DEPTH_STENCIL is not available natively, so we
> emulate it
> -    // at the command buffer level for WebGL contexts.
> -    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER,
GL_DEPTH_STENCIL_ATTACHMENT,
> GL_RENDERBUFFER, m_depthStencilBuffer);
> +    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT,
> GL_RENDERBUFFER, m_depthStencilBuffer);
> +    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT,
> GL_RENDERBUFFER, m_depthStencilBuffer);
>      m_gl->BindRenderbuffer(GL_RENDERBUFFER, 0);
>  }
> 
> There's more cleanup work to be done though. How about pulling that patch into
> yours and let's file a follow-up bug about cleaning things up in this area?

Mmh, good point...
Should we fix this properly in the command buffer code (i.e. make
GL_DEPTH_STENCIL_ATTACHMENT behave like ES3 requires, i.e. be equivalent to
binding both depth and stencil, as opposed to a separate binding)? Are there
semantics differences in the WebGL 1 version?

[Ken Russell (switch to Gerrit), 2016-09-21 01:39:03]

On 2016/09/21 00:21:08, piman OOO back 2016-09-12 wrote:
> On 2016/09/20 23:51:16, Ken Russell wrote:
> > On 2016/09/20 23:24:24, Ken Russell wrote:
> > > Sorry for the delay reviewing. The test failures look real and I can
> reproduce
> > > them locally. Investigating them now.
> > 
> > The failures are happening because WebGL 1.0 introduced a synthetic
> > GL_DEPTH_STENCIL_ATTACHMENT point distinct from GL_DEPTH_ATTACHMENT and
> > GL_STENCIL_ATTACHMENT. This was intended to allow portable support of
> > depth/stencil renderbuffers on systems that either supported separate
> > depth/stencil or packed depth/stencil. Now that Chrome only supports WebGL
1.0
> > on systems with packed depth/stencil we should simplify the code and remove
> the
> > command buffer's notion of the distinct GL_DEPTH_STENCIL_ATTACHMENT point.
> > 
> > The following added patch will make these tests pass:
> > 
> > diff --git
a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> > b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> > index a52ced0..24a8071 100644
> > --- a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> > +++ b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> > @@ -806,9 +806,8 @@ void DrawingBuffer::resizeDepthStencil(const IntSize&
> size)
> >          m_gl->RenderbufferStorageMultisampleCHROMIUM(GL_RENDERBUFFER,
> > m_sampleCount, GL_DEPTH24_STENCIL8_OES, size.width(), size.height());
> >      else
> >          m_gl->RenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH24_STENCIL8_OES,
> > size.width(), size.height());
> > -    // For ES 2.0 contexts DEPTH_STENCIL is not available natively, so we
> > emulate it
> > -    // at the command buffer level for WebGL contexts.
> > -    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER,
> GL_DEPTH_STENCIL_ATTACHMENT,
> > GL_RENDERBUFFER, m_depthStencilBuffer);
> > +    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT,
> > GL_RENDERBUFFER, m_depthStencilBuffer);
> > +    m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT,
> > GL_RENDERBUFFER, m_depthStencilBuffer);
> >      m_gl->BindRenderbuffer(GL_RENDERBUFFER, 0);
> >  }
> > 
> > There's more cleanup work to be done though. How about pulling that patch
into
> > yours and let's file a follow-up bug about cleaning things up in this area?
> 
> Mmh, good point...
> Should we fix this properly in the command buffer code (i.e. make
> GL_DEPTH_STENCIL_ATTACHMENT behave like ES3 requires, i.e. be equivalent to
> binding both depth and stencil, as opposed to a separate binding)? Are there
> semantics differences in the WebGL 1 version?

Yes, unfortunately there's a big semantic difference in WebGL 1.0 -- the
GL_DEPTH_STENCIL_ATTACHMENT point is a distinct attachment point, and defined to
be mutually exclusive with respect to GL_DEPTH_ATTACHMENT and
GL_STENCIL_ATTACHMENT.

At this point I think this can be handled entirely on the Blink side, but would
need to revisit the code and tests. Not sure that we should block your patch on
that.

[piman, 2016-09-21 22:11:26]

On Tue, Sep 20, 2016 at 6:39 PM, <kbr@chromium.org> wrote:

> On 2016/09/21 00:21:08, piman OOO back 2016-09-12 wrote:
> > On 2016/09/20 23:51:16, Ken Russell wrote:
> > > On 2016/09/20 23:24:24, Ken Russell wrote:
> > > > Sorry for the delay reviewing. The test failures look real and I can
> > reproduce
> > > > them locally. Investigating them now.
> > >
> > > The failures are happening because WebGL 1.0 introduced a synthetic
> > > GL_DEPTH_STENCIL_ATTACHMENT point distinct from GL_DEPTH_ATTACHMENT and
> > > GL_STENCIL_ATTACHMENT. This was intended to allow portable support of
> > > depth/stencil renderbuffers on systems that either supported separate
> > > depth/stencil or packed depth/stencil. Now that Chrome only supports
> WebGL
> 1.0
> > > on systems with packed depth/stencil we should simplify the code and
> remove
> > the
> > > command buffer's notion of the distinct GL_DEPTH_STENCIL_ATTACHMENT
> point.
> > >
> > > The following added patch will make these tests pass:
> > >
> > > diff --git
> a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> > > b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
> > > index a52ced0..24a8071 100644
> > > --- a/third_party/WebKit/Source/platform/graphics/gpu/
> DrawingBuffer.cpp
> > > +++ b/third_party/WebKit/Source/platform/graphics/gpu/
> DrawingBuffer.cpp
> > > @@ -806,9 +806,8 @@ void DrawingBuffer::resizeDepthStencil(const
> IntSize&
> > size)
> > > m_gl->RenderbufferStorageMultisampleCHROMIUM(GL_RENDERBUFFER,
> > > m_sampleCount, GL_DEPTH24_STENCIL8_OES, size.width(), size.height());
> > > else
> > > m_gl->RenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH24_STENCIL8_OES,
> > > size.width(), size.height());
> > > - // For ES 2.0 contexts DEPTH_STENCIL is not available natively, so we
> > > emulate it
> > > - // at the command buffer level for WebGL contexts.
> > > - m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER,
> > GL_DEPTH_STENCIL_ATTACHMENT,
> > > GL_RENDERBUFFER, m_depthStencilBuffer);
> > > + m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT,
> > > GL_RENDERBUFFER, m_depthStencilBuffer);
> > > + m_gl->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT,
> > > GL_RENDERBUFFER, m_depthStencilBuffer);
> > > m_gl->BindRenderbuffer(GL_RENDERBUFFER, 0);
> > > }
> > >
> > > There's more cleanup work to be done though. How about pulling that
> patch
> into
> > > yours and let's file a follow-up bug about cleaning things up in this
> area?
> >
> > Mmh, good point...
> > Should we fix this properly in the command buffer code (i.e. make
> > GL_DEPTH_STENCIL_ATTACHMENT behave like ES3 requires, i.e. be equivalent
> to
> > binding both depth and stencil, as opposed to a separate binding)? Are
> there
> > semantics differences in the WebGL 1 version?
>
> Yes, unfortunately there's a big semantic difference in WebGL 1.0 -- the
> GL_DEPTH_STENCIL_ATTACHMENT point is a distinct attachment point, and
> defined to
> be mutually exclusive with respect to GL_DEPTH_ATTACHMENT and
> GL_STENCIL_ATTACHMENT.
>
> At this point I think this can be handled entirely on the Blink side, but
> would
> need to revisit the code and tests. Not sure that we should block your
> patch on
> that.
>

Thanks, I tried to apply your patch, but it breaks WebGL, because there are
additional checks for WebGL1 contexts, that verify that DEPTH_STENCIL
resources are only bound to DEPTH_STENCIL_ATTACHMENT points. Let me see if
I can find a short term solution, but I agree, I think we should eventually
move handling that special attachment point to the Blink side, because as
is it makes the security validation harder (and breaks ES3 semantics in
subtle ways).


>
> https://codereview.chromium.org/2351093002/
>

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

[piman, 2016-09-21 23:16:20]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 23:16:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-09-21 23:17:22]

PTAL, I fixed the logic to look at both DEPTH_ATTACHMENT and
DEPTH_STENCIL_ATTACHMENT points (and same with stencil), and checked locally
that the WebGL issues are gone.

[Ken Russell (switch to Gerrit), 2016-09-21 23:44:43]

Thanks for working around the mess there. LGTM

[piman, 2016-09-21 23:45:40]

The CQ bit was unchecked by piman@chromium.org

[piman, 2016-09-21 23:45:45]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2016-09-21 23:46:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 00:04:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-22 00:31:32]

Description was changed from

==========
Fix crash in BlitFramebufferCHROMIUM if a read or draw depth/stencil buffer is
not present

From ES3 spec: "If a buffer is specified in mask and does not exist in both the
read and draw
framebuffers, the corresponding bit is silently ignored."

BUG=648133
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix crash in BlitFramebufferCHROMIUM if a read or draw depth/stencil buffer is
not present

From ES3 spec: "If a buffer is specified in mask and does not exist in both the
read and draw
framebuffers, the corresponding bit is silently ignored."

BUG=648133
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/72e9876bdadd8cb4df5e813d03b5ca8d966d214f
Cr-Commit-Position: refs/heads/master@{#420207}
==========

[commit-bot: I haz the power, 2016-09-22 00:31:33]

Patchset 4 (id:??) landed as
https://crrev.com/72e9876bdadd8cb4df5e813d03b5ca8d966d214f
Cr-Commit-Position: refs/heads/master@{#420207}