Issue 2149523002: Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment

Jiawei

Description was changed from ========== Add depth_cleared_ and stencil_cleared_ for class texture and renderbuffer. Textures ...

4 years, 5 months ago (2016-07-13 08:54:56 UTC) #1

Jiawei

Description was changed from ========== Add depth_cleared_ and stencil_cleared_ for class texture and renderbuffer. Textures ...

4 years, 5 months ago (2016-07-14 00:22:49 UTC) #2

Jiawei

jiawei.shao@intel.com changed reviewers: + kbr@chromium.org, qiankun.miao@intel.com, yunchao.he@intel.com, zmo@chromium.org

4 years, 5 months ago (2016-07-14 00:25:19 UTC) #3

qiankun

Please format description by "edit issue" on top-left. Current implementation can fix bugs in InvalidateFramebuffer ...

4 years, 5 months ago (2016-07-14 06:48:22 UTC) #5

Jiawei

Description was changed from ========== Add depth_cleared_ and stencil_cleared_ for class texture and renderbuffer. Textures ...

4 years, 5 months ago (2016-07-14 06:59:26 UTC) #6

qiankun

jiawei: please fix the try-bot failure. kbr, zmo: can you give a glance at this ...

4 years, 5 months ago (2016-07-14 14:35:33 UTC) #7

Zhenyao Mo

First, let me clarify this is a bug when we initialize uninitialized DEPTH_STENCIL resources? In ...

4 years, 5 months ago (2016-07-14 22:48:14 UTC) #8

Ken Russell (switch to Gerrit)

On 2016/07/14 22:48:14, Zhenyao Mo wrote: > First, let me clarify this is a bug ...

4 years, 5 months ago (2016-07-15 01:01:11 UTC) #9

Jiawei

On 2016/07/15 01:01:11, Ken Russell wrote: > On 2016/07/14 22:48:14, Zhenyao Mo wrote: > > ...

4 years, 5 months ago (2016-07-15 01:12:18 UTC) #10

Jiawei

On 2016/07/14 22:48:14, Zhenyao Mo wrote: > First, let me clarify this is a bug ...

4 years, 5 months ago (2016-07-15 01:20:26 UTC) #13

On 2016/07/14 22:48:14, Zhenyao Mo wrote:
> First, let me clarify this is a bug when we initialize uninitialized
> DEPTH_STENCIL resources?
> 
> In a high level (without looking too much details into the CL), what are the
> situations that we ended up clear DEPTH without STENCIL or vice versa? On top
of
> my head, it is only the cases where you attach a DEPTH_STENCIL to only
> DEPTH_ATTACHMENT or STENCIL_ATTACHMENT, but not both.  Is that the case?  If
> yes, then we simply need to fix that in ClearUnclearedAttachment() function,
> which is much less intrusive.
> 
> But if I miss the point, please help me understand.

OK. Here is all my investigation on this issue. I hope it can make some help to
you.

Let me use texture as an example, and the situation of renderbuffer is same. The
bug I find is that in the previous implementation, we can only mark a texture
cleared or uncleared as a whole, as there is only one variable cleared_ in the
texture object.

You can pay attention on the function InvalidateFramebufferImpl() in
gles2_cmd_decoder.cc, which is the implementation of WebGL API
invalidateFramebuffer(). This function calls
framebuffer->MarkAttachmentAsCleared(), which is intended to mark the specific
attachment of the framebuffer cleared or uncleared. 

The class TextureAttachment represents a specific attachment of a framebuffer,
and it wraps the texture attached to the framebuffer. There is a pointer-like
variable called texture_ref which points to the texture. The texture with format
DEPTH_STENCIL should be attached to both DEPTH_ATTACHMENT and STENCIL_ATTACHMENT
in the same framebuffer when you use textures with this format as depth and
stencil attachments, so texture_refs in both depth TextureAttachment and stencil
TextureAttachment point to the same texture with format DEPTH_STENCIL. You can
verify this fact by checking the address of texture_ref->texture() in these two
TextureAttachment objects. The structure of the framebuffer in this situation is
as follows:

DEPTH_ATTACHMENT                   STENCIL_ATTACHMENT
       |                                   |
TextureAttachment                  TextureAttachment
        \                                 /
         \                               /
          Texture in format DEPTH_STENCIL

When framebuffer->MarkAttachmentAsCleared() is called, function SetCleared() in
the specific TextureAttachment object is called, and finally function
SetLevelCleared() in the Texture object is called by TextureManager to update
cleared_ in the Texture object. 

In the failed case invalidate.whole.unbind_read_stencil, only STENCIL_ATTACHMENT
of the framebuffer should be invalidated and cleared, and information in
DEPTH_ATTACHMENT should be kept. But in current implementation, when you intend
to mark STENCIL_ATTACHMENT, you can only call the function SetLevelCleared() to
mark the Texture as a whole by updating the variable cleared_. This can be
proved after you call framebuffer->MarkAttachmentAsCleared(..., STENCIL_BUFFER),
you can call framebuffer -> HasUnclearedAttachment(DEPTH_ATTACHMENT) and
framebuffer->HasUnclearedAttachment(STENCIL_ATTACHMENT), and the return values
will be both true because the variable cleared_ in the Texture object is true.

It is not enough just to fix ClearUnclearedAttachment(). You can see function()
DoDrawArrays() or DoDrawElements() in gles2_cmd_decoder.cc. Before really
calling glDrawElements() this function will call ClearUnclearedTextures() at
first to make sure there is no uncertain values in all textures.
ClearUnclearedTextures() checks all the textures and call
texture_manager()->ClearRenderableLevels(), which will finally call the function
ClearLevel() in gles2_cmd_decoder.cc. In the previous implementation, the
function ClearLevel() will just clear every part of the texture by
glClear(GL_DEPTH_BUTTER_BIT | GL_STENCIL_BUFFER_BIT). In the situation before,
as the texture has been marked as uncleared, ClearLevel() is called to clear the
texture, so the depth part, which should not be cleared, is cleared too.

So I think the root cause of the bug is that in the previous implementation
Textures with format DEPTH_STENCIL fail to describe the state when only a part
of the texture is uncleared, and another part is cleared, so I am trying to fix
this issue.

Zhenyao Mo

On 2016/07/15 01:20:26, Jiawei wrote: > On 2016/07/14 22:48:14, Zhenyao Mo wrote: > > First, ...

4 years, 5 months ago (2016-07-15 01:26:25 UTC) #14

On 2016/07/15 01:20:26, Jiawei wrote:
> On 2016/07/14 22:48:14, Zhenyao Mo wrote:
> > First, let me clarify this is a bug when we initialize uninitialized
> > DEPTH_STENCIL resources?
> > 
> > In a high level (without looking too much details into the CL), what are the
> > situations that we ended up clear DEPTH without STENCIL or vice versa? On
top
> of
> > my head, it is only the cases where you attach a DEPTH_STENCIL to only
> > DEPTH_ATTACHMENT or STENCIL_ATTACHMENT, but not both.  Is that the case?  If
> > yes, then we simply need to fix that in ClearUnclearedAttachment() function,
> > which is much less intrusive.
> > 
> > But if I miss the point, please help me understand.
> 
> OK. Here is all my investigation on this issue. I hope it can make some help
to
> you.
> 
> Let me use texture as an example, and the situation of renderbuffer is same.
The
> bug I find is that in the previous implementation, we can only mark a texture
> cleared or uncleared as a whole, as there is only one variable cleared_ in the
> texture object.
> 
> You can pay attention on the function InvalidateFramebufferImpl() in
> gles2_cmd_decoder.cc, which is the implementation of WebGL API
> invalidateFramebuffer(). This function calls
> framebuffer->MarkAttachmentAsCleared(), which is intended to mark the specific
> attachment of the framebuffer cleared or uncleared. 
> 
> The class TextureAttachment represents a specific attachment of a framebuffer,
> and it wraps the texture attached to the framebuffer. There is a pointer-like
> variable called texture_ref which points to the texture. The texture with
format
> DEPTH_STENCIL should be attached to both DEPTH_ATTACHMENT and
STENCIL_ATTACHMENT
> in the same framebuffer when you use textures with this format as depth and
> stencil attachments, so texture_refs in both depth TextureAttachment and
stencil
> TextureAttachment point to the same texture with format DEPTH_STENCIL. You can
> verify this fact by checking the address of texture_ref->texture() in these
two
> TextureAttachment objects. The structure of the framebuffer in this situation
is
> as follows:
> 
> DEPTH_ATTACHMENT                   STENCIL_ATTACHMENT
>        |                                   |
> TextureAttachment                  TextureAttachment
>         \                                 /
>          \                               /
>           Texture in format DEPTH_STENCIL
> 
> When framebuffer->MarkAttachmentAsCleared() is called, function SetCleared()
in
> the specific TextureAttachment object is called, and finally function
> SetLevelCleared() in the Texture object is called by TextureManager to update
> cleared_ in the Texture object. 
> 
> In the failed case invalidate.whole.unbind_read_stencil, only
STENCIL_ATTACHMENT
> of the framebuffer should be invalidated and cleared, and information in
> DEPTH_ATTACHMENT should be kept. But in current implementation, when you
intend
> to mark STENCIL_ATTACHMENT, you can only call the function SetLevelCleared()
to
> mark the Texture as a whole by updating the variable cleared_. This can be
> proved after you call framebuffer->MarkAttachmentAsCleared(...,
STENCIL_BUFFER),
> you can call framebuffer -> HasUnclearedAttachment(DEPTH_ATTACHMENT) and
> framebuffer->HasUnclearedAttachment(STENCIL_ATTACHMENT), and the return values
> will be both true because the variable cleared_ in the Texture object is true.
> 
> It is not enough just to fix ClearUnclearedAttachment(). You can see
function()
> DoDrawArrays() or DoDrawElements() in gles2_cmd_decoder.cc. Before really
> calling glDrawElements() this function will call ClearUnclearedTextures() at
> first to make sure there is no uncertain values in all textures.
> ClearUnclearedTextures() checks all the textures and call
> texture_manager()->ClearRenderableLevels(), which will finally call the
function
> ClearLevel() in gles2_cmd_decoder.cc. In the previous implementation, the
> function ClearLevel() will just clear every part of the texture by
> glClear(GL_DEPTH_BUTTER_BIT | GL_STENCIL_BUFFER_BIT). In the situation before,
> as the texture has been marked as uncleared, ClearLevel() is called to clear
the
> texture, so the depth part, which should not be cleared, is cleared too.
> 
> So I think the root cause of the bug is that in the previous implementation
> Textures with format DEPTH_STENCIL fail to describe the state when only a part
> of the texture is uncleared, and another part is cleared, so I am trying to
fix
> this issue.

Thanks for the detailed explanation. I understand now.  So there are only two
situations that we might have an uncleared image. 1) it's allocated, but not
initialized (texImage with null data, renderbuffer) 2) we invalidate it

So for 1), we will never clear depth without clearing stencil (or vice versa),
so no need to worry about it.

for 2), yes, the current implementation is buggy and I still think avoid
InvalidateFramebuffer to the lower driver when we have DEPTH_STENCIL image and
we only invalidate depth or stencil bit.

Please let me know if you think this might work.

Zhenyao Mo

On 2016/07/15 01:12:18, Jiawei wrote: > On 2016/07/15 01:01:11, Ken Russell wrote: > > On ...

4 years, 5 months ago (2016-07-15 01:39:06 UTC) #15

Jiawei

On 2016/07/15 01:26:25, Zhenyao Mo wrote: > On 2016/07/15 01:20:26, Jiawei wrote: > > On ...

4 years, 5 months ago (2016-07-15 01:45:23 UTC) #16

On 2016/07/15 01:26:25, Zhenyao Mo wrote:
> On 2016/07/15 01:20:26, Jiawei wrote:
> > On 2016/07/14 22:48:14, Zhenyao Mo wrote:
> > > First, let me clarify this is a bug when we initialize uninitialized
> > > DEPTH_STENCIL resources?
> > > 
> > > In a high level (without looking too much details into the CL), what are
the
> > > situations that we ended up clear DEPTH without STENCIL or vice versa? On
> top
> > of
> > > my head, it is only the cases where you attach a DEPTH_STENCIL to only
> > > DEPTH_ATTACHMENT or STENCIL_ATTACHMENT, but not both.  Is that the case? 
If
> > > yes, then we simply need to fix that in ClearUnclearedAttachment()
function,
> > > which is much less intrusive.
> > > 
> > > But if I miss the point, please help me understand.
> > 
> > OK. Here is all my investigation on this issue. I hope it can make some help
> to
> > you.
> > 
> > Let me use texture as an example, and the situation of renderbuffer is same.
> The
> > bug I find is that in the previous implementation, we can only mark a
texture
> > cleared or uncleared as a whole, as there is only one variable cleared_ in
the
> > texture object.
> > 
> > You can pay attention on the function InvalidateFramebufferImpl() in
> > gles2_cmd_decoder.cc, which is the implementation of WebGL API
> > invalidateFramebuffer(). This function calls
> > framebuffer->MarkAttachmentAsCleared(), which is intended to mark the
specific
> > attachment of the framebuffer cleared or uncleared. 
> > 
> > The class TextureAttachment represents a specific attachment of a
framebuffer,
> > and it wraps the texture attached to the framebuffer. There is a
pointer-like
> > variable called texture_ref which points to the texture. The texture with
> format
> > DEPTH_STENCIL should be attached to both DEPTH_ATTACHMENT and
> STENCIL_ATTACHMENT
> > in the same framebuffer when you use textures with this format as depth and
> > stencil attachments, so texture_refs in both depth TextureAttachment and
> stencil
> > TextureAttachment point to the same texture with format DEPTH_STENCIL. You
can
> > verify this fact by checking the address of texture_ref->texture() in these
> two
> > TextureAttachment objects. The structure of the framebuffer in this
situation
> is
> > as follows:
> > 
> > DEPTH_ATTACHMENT                   STENCIL_ATTACHMENT
> >        |                                   |
> > TextureAttachment                  TextureAttachment
> >         \                                 /
> >          \                               /
> >           Texture in format DEPTH_STENCIL
> > 
> > When framebuffer->MarkAttachmentAsCleared() is called, function SetCleared()
> in
> > the specific TextureAttachment object is called, and finally function
> > SetLevelCleared() in the Texture object is called by TextureManager to
update
> > cleared_ in the Texture object. 
> > 
> > In the failed case invalidate.whole.unbind_read_stencil, only
> STENCIL_ATTACHMENT
> > of the framebuffer should be invalidated and cleared, and information in
> > DEPTH_ATTACHMENT should be kept. But in current implementation, when you
> intend
> > to mark STENCIL_ATTACHMENT, you can only call the function SetLevelCleared()
> to
> > mark the Texture as a whole by updating the variable cleared_. This can be
> > proved after you call framebuffer->MarkAttachmentAsCleared(...,
> STENCIL_BUFFER),
> > you can call framebuffer -> HasUnclearedAttachment(DEPTH_ATTACHMENT) and
> > framebuffer->HasUnclearedAttachment(STENCIL_ATTACHMENT), and the return
values
> > will be both true because the variable cleared_ in the Texture object is
true.
> > 
> > It is not enough just to fix ClearUnclearedAttachment(). You can see
> function()
> > DoDrawArrays() or DoDrawElements() in gles2_cmd_decoder.cc. Before really
> > calling glDrawElements() this function will call ClearUnclearedTextures() at
> > first to make sure there is no uncertain values in all textures.
> > ClearUnclearedTextures() checks all the textures and call
> > texture_manager()->ClearRenderableLevels(), which will finally call the
> function
> > ClearLevel() in gles2_cmd_decoder.cc. In the previous implementation, the
> > function ClearLevel() will just clear every part of the texture by
> > glClear(GL_DEPTH_BUTTER_BIT | GL_STENCIL_BUFFER_BIT). In the situation
before,
> > as the texture has been marked as uncleared, ClearLevel() is called to clear
> the
> > texture, so the depth part, which should not be cleared, is cleared too.
> > 
> > So I think the root cause of the bug is that in the previous implementation
> > Textures with format DEPTH_STENCIL fail to describe the state when only a
part
> > of the texture is uncleared, and another part is cleared, so I am trying to
> fix
> > this issue.
> 
> Thanks for the detailed explanation. I understand now.  So there are only two
> situations that we might have an uncleared image. 1) it's allocated, but not
> initialized (texImage with null data, renderbuffer) 2) we invalidate it
> 
> So for 1), we will never clear depth without clearing stencil (or vice versa),
> so no need to worry about it.
> 
> for 2), yes, the current implementation is buggy and I still think avoid
> InvalidateFramebuffer to the lower driver when we have DEPTH_STENCIL image and
> we only invalidate depth or stencil bit.
> 
> Please let me know if you think this might work.

For 2), I think the driver can do the right thing to deal with DEPTH_STENCIL
image. You can prove that by commenting the code that marks the attachment as
cleared in invalidateFramebufferImpl(), tests in fboinvalidate can pass, but
when you try to draw the invalidated attachment, you can see there is uncertain
information in the texture. 
I fully agree with you and I think we can use glClears to implement
invalidateFramebuffer() instead of calling drivers. Maybe we can also use this
way to implement invalidateSubFramebuffer() which is still an empty function
now. 

Thanks for your advice. I will try it in this way.

Zhenyao Mo

On 2016/07/15 01:45:23, Jiawei wrote: > On 2016/07/15 01:26:25, Zhenyao Mo wrote: > > On ...

4 years, 5 months ago (2016-07-15 01:51:43 UTC) #17

On 2016/07/15 01:45:23, Jiawei wrote:
> On 2016/07/15 01:26:25, Zhenyao Mo wrote:
> > On 2016/07/15 01:20:26, Jiawei wrote:
> > > On 2016/07/14 22:48:14, Zhenyao Mo wrote:
> > > > First, let me clarify this is a bug when we initialize uninitialized
> > > > DEPTH_STENCIL resources?
> > > > 
> > > > In a high level (without looking too much details into the CL), what are
> the
> > > > situations that we ended up clear DEPTH without STENCIL or vice versa?
On
> > top
> > > of
> > > > my head, it is only the cases where you attach a DEPTH_STENCIL to only
> > > > DEPTH_ATTACHMENT or STENCIL_ATTACHMENT, but not both.  Is that the case?

> If
> > > > yes, then we simply need to fix that in ClearUnclearedAttachment()
> function,
> > > > which is much less intrusive.
> > > > 
> > > > But if I miss the point, please help me understand.
> > > 
> > > OK. Here is all my investigation on this issue. I hope it can make some
help
> > to
> > > you.
> > > 
> > > Let me use texture as an example, and the situation of renderbuffer is
same.
> > The
> > > bug I find is that in the previous implementation, we can only mark a
> texture
> > > cleared or uncleared as a whole, as there is only one variable cleared_ in
> the
> > > texture object.
> > > 
> > > You can pay attention on the function InvalidateFramebufferImpl() in
> > > gles2_cmd_decoder.cc, which is the implementation of WebGL API
> > > invalidateFramebuffer(). This function calls
> > > framebuffer->MarkAttachmentAsCleared(), which is intended to mark the
> specific
> > > attachment of the framebuffer cleared or uncleared. 
> > > 
> > > The class TextureAttachment represents a specific attachment of a
> framebuffer,
> > > and it wraps the texture attached to the framebuffer. There is a
> pointer-like
> > > variable called texture_ref which points to the texture. The texture with
> > format
> > > DEPTH_STENCIL should be attached to both DEPTH_ATTACHMENT and
> > STENCIL_ATTACHMENT
> > > in the same framebuffer when you use textures with this format as depth
and
> > > stencil attachments, so texture_refs in both depth TextureAttachment and
> > stencil
> > > TextureAttachment point to the same texture with format DEPTH_STENCIL. You
> can
> > > verify this fact by checking the address of texture_ref->texture() in
these
> > two
> > > TextureAttachment objects. The structure of the framebuffer in this
> situation
> > is
> > > as follows:
> > > 
> > > DEPTH_ATTACHMENT                   STENCIL_ATTACHMENT
> > >        |                                   |
> > > TextureAttachment                  TextureAttachment
> > >         \                                 /
> > >          \                               /
> > >           Texture in format DEPTH_STENCIL
> > > 
> > > When framebuffer->MarkAttachmentAsCleared() is called, function
SetCleared()
> > in
> > > the specific TextureAttachment object is called, and finally function
> > > SetLevelCleared() in the Texture object is called by TextureManager to
> update
> > > cleared_ in the Texture object. 
> > > 
> > > In the failed case invalidate.whole.unbind_read_stencil, only
> > STENCIL_ATTACHMENT
> > > of the framebuffer should be invalidated and cleared, and information in
> > > DEPTH_ATTACHMENT should be kept. But in current implementation, when you
> > intend
> > > to mark STENCIL_ATTACHMENT, you can only call the function
SetLevelCleared()
> > to
> > > mark the Texture as a whole by updating the variable cleared_. This can be
> > > proved after you call framebuffer->MarkAttachmentAsCleared(...,
> > STENCIL_BUFFER),
> > > you can call framebuffer -> HasUnclearedAttachment(DEPTH_ATTACHMENT) and
> > > framebuffer->HasUnclearedAttachment(STENCIL_ATTACHMENT), and the return
> values
> > > will be both true because the variable cleared_ in the Texture object is
> true.
> > > 
> > > It is not enough just to fix ClearUnclearedAttachment(). You can see
> > function()
> > > DoDrawArrays() or DoDrawElements() in gles2_cmd_decoder.cc. Before really
> > > calling glDrawElements() this function will call ClearUnclearedTextures()
at
> > > first to make sure there is no uncertain values in all textures.
> > > ClearUnclearedTextures() checks all the textures and call
> > > texture_manager()->ClearRenderableLevels(), which will finally call the
> > function
> > > ClearLevel() in gles2_cmd_decoder.cc. In the previous implementation, the
> > > function ClearLevel() will just clear every part of the texture by
> > > glClear(GL_DEPTH_BUTTER_BIT | GL_STENCIL_BUFFER_BIT). In the situation
> before,
> > > as the texture has been marked as uncleared, ClearLevel() is called to
clear
> > the
> > > texture, so the depth part, which should not be cleared, is cleared too.
> > > 
> > > So I think the root cause of the bug is that in the previous
implementation
> > > Textures with format DEPTH_STENCIL fail to describe the state when only a
> part
> > > of the texture is uncleared, and another part is cleared, so I am trying
to
> > fix
> > > this issue.
> > 
> > Thanks for the detailed explanation. I understand now.  So there are only
two
> > situations that we might have an uncleared image. 1) it's allocated, but not
> > initialized (texImage with null data, renderbuffer) 2) we invalidate it
> > 
> > So for 1), we will never clear depth without clearing stencil (or vice
versa),
> > so no need to worry about it.
> > 
> > for 2), yes, the current implementation is buggy and I still think avoid
> > InvalidateFramebuffer to the lower driver when we have DEPTH_STENCIL image
and
> > we only invalidate depth or stencil bit.
> > 
> > Please let me know if you think this might work.
> 
> For 2), I think the driver can do the right thing to deal with DEPTH_STENCIL
> image. You can prove that by commenting the code that marks the attachment as
> cleared in invalidateFramebufferImpl(), tests in fboinvalidate can pass, but
> when you try to draw the invalidated attachment, you can see there is
uncertain
> information in the texture. 
> I fully agree with you and I think we can use glClears to implement
> invalidateFramebuffer() instead of calling drivers. Maybe we can also use this
> way to implement invalidateSubFramebuffer() which is still an empty function
> now. 
> 
> Thanks for your advice. I will try it in this way.

I don't think we should do clear. It's actually go against the idea of
InvalidateFramebuffer, which allows drivers to optimize.  It's OK for previous
contents from the same context to stay, no need to clear them.  We only
concerned about undefiend behavior, where potentially some vram from other
contexts could sneak in and leak sensitive information.  I think just leaving
the above InvalidateFramebuffer case and InvalidateSubFramebuffer as no-op is
acceptable.  It is also spec compliant, because Spec says undefined contents,
and leaving image unchanged is one of the undefined behavior.

Jiawei

On 2016/07/15 01:51:43, Zhenyao Mo wrote: > On 2016/07/15 01:45:23, Jiawei wrote: > > On ...

4 years, 5 months ago (2016-07-15 02:00:29 UTC) #18

On 2016/07/15 01:51:43, Zhenyao Mo wrote:
> On 2016/07/15 01:45:23, Jiawei wrote:
> > On 2016/07/15 01:26:25, Zhenyao Mo wrote:
> > > On 2016/07/15 01:20:26, Jiawei wrote:
> > > > On 2016/07/14 22:48:14, Zhenyao Mo wrote:
> > > > > First, let me clarify this is a bug when we initialize uninitialized
> > > > > DEPTH_STENCIL resources?
> > > > > 
> > > > > In a high level (without looking too much details into the CL), what
are
> > the
> > > > > situations that we ended up clear DEPTH without STENCIL or vice versa?
> On
> > > top
> > > > of
> > > > > my head, it is only the cases where you attach a DEPTH_STENCIL to only
> > > > > DEPTH_ATTACHMENT or STENCIL_ATTACHMENT, but not both.  Is that the
case?
> 
> > If
> > > > > yes, then we simply need to fix that in ClearUnclearedAttachment()
> > function,
> > > > > which is much less intrusive.
> > > > > 
> > > > > But if I miss the point, please help me understand.
> > > > 
> > > > OK. Here is all my investigation on this issue. I hope it can make some
> help
> > > to
> > > > you.
> > > > 
> > > > Let me use texture as an example, and the situation of renderbuffer is
> same.
> > > The
> > > > bug I find is that in the previous implementation, we can only mark a
> > texture
> > > > cleared or uncleared as a whole, as there is only one variable cleared_
in
> > the
> > > > texture object.
> > > > 
> > > > You can pay attention on the function InvalidateFramebufferImpl() in
> > > > gles2_cmd_decoder.cc, which is the implementation of WebGL API
> > > > invalidateFramebuffer(). This function calls
> > > > framebuffer->MarkAttachmentAsCleared(), which is intended to mark the
> > specific
> > > > attachment of the framebuffer cleared or uncleared. 
> > > > 
> > > > The class TextureAttachment represents a specific attachment of a
> > framebuffer,
> > > > and it wraps the texture attached to the framebuffer. There is a
> > pointer-like
> > > > variable called texture_ref which points to the texture. The texture
with
> > > format
> > > > DEPTH_STENCIL should be attached to both DEPTH_ATTACHMENT and
> > > STENCIL_ATTACHMENT
> > > > in the same framebuffer when you use textures with this format as depth
> and
> > > > stencil attachments, so texture_refs in both depth TextureAttachment and
> > > stencil
> > > > TextureAttachment point to the same texture with format DEPTH_STENCIL.
You
> > can
> > > > verify this fact by checking the address of texture_ref->texture() in
> these
> > > two
> > > > TextureAttachment objects. The structure of the framebuffer in this
> > situation
> > > is
> > > > as follows:
> > > > 
> > > > DEPTH_ATTACHMENT                   STENCIL_ATTACHMENT
> > > >        |                                   |
> > > > TextureAttachment                  TextureAttachment
> > > >         \                                 /
> > > >          \                               /
> > > >           Texture in format DEPTH_STENCIL
> > > > 
> > > > When framebuffer->MarkAttachmentAsCleared() is called, function
> SetCleared()
> > > in
> > > > the specific TextureAttachment object is called, and finally function
> > > > SetLevelCleared() in the Texture object is called by TextureManager to
> > update
> > > > cleared_ in the Texture object. 
> > > > 
> > > > In the failed case invalidate.whole.unbind_read_stencil, only
> > > STENCIL_ATTACHMENT
> > > > of the framebuffer should be invalidated and cleared, and information in
> > > > DEPTH_ATTACHMENT should be kept. But in current implementation, when you
> > > intend
> > > > to mark STENCIL_ATTACHMENT, you can only call the function
> SetLevelCleared()
> > > to
> > > > mark the Texture as a whole by updating the variable cleared_. This can
be
> > > > proved after you call framebuffer->MarkAttachmentAsCleared(...,
> > > STENCIL_BUFFER),
> > > > you can call framebuffer -> HasUnclearedAttachment(DEPTH_ATTACHMENT) and
> > > > framebuffer->HasUnclearedAttachment(STENCIL_ATTACHMENT), and the return
> > values
> > > > will be both true because the variable cleared_ in the Texture object is
> > true.
> > > > 
> > > > It is not enough just to fix ClearUnclearedAttachment(). You can see
> > > function()
> > > > DoDrawArrays() or DoDrawElements() in gles2_cmd_decoder.cc. Before
really
> > > > calling glDrawElements() this function will call
ClearUnclearedTextures()
> at
> > > > first to make sure there is no uncertain values in all textures.
> > > > ClearUnclearedTextures() checks all the textures and call
> > > > texture_manager()->ClearRenderableLevels(), which will finally call the
> > > function
> > > > ClearLevel() in gles2_cmd_decoder.cc. In the previous implementation,
the
> > > > function ClearLevel() will just clear every part of the texture by
> > > > glClear(GL_DEPTH_BUTTER_BIT | GL_STENCIL_BUFFER_BIT). In the situation
> > before,
> > > > as the texture has been marked as uncleared, ClearLevel() is called to
> clear
> > > the
> > > > texture, so the depth part, which should not be cleared, is cleared too.
> > > > 
> > > > So I think the root cause of the bug is that in the previous
> implementation
> > > > Textures with format DEPTH_STENCIL fail to describe the state when only
a
> > part
> > > > of the texture is uncleared, and another part is cleared, so I am trying
> to
> > > fix
> > > > this issue.
> > > 
> > > Thanks for the detailed explanation. I understand now.  So there are only
> two
> > > situations that we might have an uncleared image. 1) it's allocated, but
not
> > > initialized (texImage with null data, renderbuffer) 2) we invalidate it
> > > 
> > > So for 1), we will never clear depth without clearing stencil (or vice
> versa),
> > > so no need to worry about it.
> > > 
> > > for 2), yes, the current implementation is buggy and I still think avoid
> > > InvalidateFramebuffer to the lower driver when we have DEPTH_STENCIL image
> and
> > > we only invalidate depth or stencil bit.
> > > 
> > > Please let me know if you think this might work.
> > 
> > For 2), I think the driver can do the right thing to deal with DEPTH_STENCIL
> > image. You can prove that by commenting the code that marks the attachment
as
> > cleared in invalidateFramebufferImpl(), tests in fboinvalidate can pass, but
> > when you try to draw the invalidated attachment, you can see there is
> uncertain
> > information in the texture. 
> > I fully agree with you and I think we can use glClears to implement
> > invalidateFramebuffer() instead of calling drivers. Maybe we can also use
this
> > way to implement invalidateSubFramebuffer() which is still an empty function
> > now. 
> > 
> > Thanks for your advice. I will try it in this way.
> 
> I don't think we should do clear. It's actually go against the idea of
> InvalidateFramebuffer, which allows drivers to optimize.  It's OK for previous
> contents from the same context to stay, no need to clear them.  We only
> concerned about undefiend behavior, where potentially some vram from other
> contexts could sneak in and leak sensitive information.  I think just leaving
> the above InvalidateFramebuffer case and InvalidateSubFramebuffer as no-op is
> acceptable.  It is also spec compliant, because Spec says undefined contents,
> and leaving image unchanged is one of the undefined behavior.

But besides fboinvalidate case, I still think it unacceptable that when the
format of the texture is DEPTH_STENCIL, after you call
framebuffer->MarkAttachmentAsCleared(...,STENCIL_BUFFER), you call framebuffer
-> HasUnclearedAttachment(DEPTH_ATTACHMENT) and
framebuffer->HasUnclearedAttachment(STENCIL_ATTACHMENT), and the return values
will be both true as the variable cleared_ in the Texture object is false. 

I think we still need to deal with this situation since it may cause some other
potential failures.

Ken Russell (switch to Gerrit)

On 2016/07/15 01:51:43, Zhenyao Mo wrote: > On 2016/07/15 01:45:23, Jiawei wrote: > > On ...

4 years, 5 months ago (2016-07-15 02:02:00 UTC) #19

On 2016/07/15 01:51:43, Zhenyao Mo wrote:
> On 2016/07/15 01:45:23, Jiawei wrote:
> > On 2016/07/15 01:26:25, Zhenyao Mo wrote:
> > > On 2016/07/15 01:20:26, Jiawei wrote:
> > > > On 2016/07/14 22:48:14, Zhenyao Mo wrote:
> > > > > First, let me clarify this is a bug when we initialize uninitialized
> > > > > DEPTH_STENCIL resources?
> > > > > 
> > > > > In a high level (without looking too much details into the CL), what
are
> > the
> > > > > situations that we ended up clear DEPTH without STENCIL or vice versa?
> On
> > > top
> > > > of
> > > > > my head, it is only the cases where you attach a DEPTH_STENCIL to only
> > > > > DEPTH_ATTACHMENT or STENCIL_ATTACHMENT, but not both.  Is that the
case?
> 
> > If
> > > > > yes, then we simply need to fix that in ClearUnclearedAttachment()
> > function,
> > > > > which is much less intrusive.
> > > > > 
> > > > > But if I miss the point, please help me understand.
> > > > 
> > > > OK. Here is all my investigation on this issue. I hope it can make some
> help
> > > to
> > > > you.
> > > > 
> > > > Let me use texture as an example, and the situation of renderbuffer is
> same.
> > > The
> > > > bug I find is that in the previous implementation, we can only mark a
> > texture
> > > > cleared or uncleared as a whole, as there is only one variable cleared_
in
> > the
> > > > texture object.
> > > > 
> > > > You can pay attention on the function InvalidateFramebufferImpl() in
> > > > gles2_cmd_decoder.cc, which is the implementation of WebGL API
> > > > invalidateFramebuffer(). This function calls
> > > > framebuffer->MarkAttachmentAsCleared(), which is intended to mark the
> > specific
> > > > attachment of the framebuffer cleared or uncleared. 
> > > > 
> > > > The class TextureAttachment represents a specific attachment of a
> > framebuffer,
> > > > and it wraps the texture attached to the framebuffer. There is a
> > pointer-like
> > > > variable called texture_ref which points to the texture. The texture
with
> > > format
> > > > DEPTH_STENCIL should be attached to both DEPTH_ATTACHMENT and
> > > STENCIL_ATTACHMENT
> > > > in the same framebuffer when you use textures with this format as depth
> and
> > > > stencil attachments, so texture_refs in both depth TextureAttachment and
> > > stencil
> > > > TextureAttachment point to the same texture with format DEPTH_STENCIL.
You
> > can
> > > > verify this fact by checking the address of texture_ref->texture() in
> these
> > > two
> > > > TextureAttachment objects. The structure of the framebuffer in this
> > situation
> > > is
> > > > as follows:
> > > > 
> > > > DEPTH_ATTACHMENT                   STENCIL_ATTACHMENT
> > > >        |                                   |
> > > > TextureAttachment                  TextureAttachment
> > > >         \                                 /
> > > >          \                               /
> > > >           Texture in format DEPTH_STENCIL
> > > > 
> > > > When framebuffer->MarkAttachmentAsCleared() is called, function
> SetCleared()
> > > in
> > > > the specific TextureAttachment object is called, and finally function
> > > > SetLevelCleared() in the Texture object is called by TextureManager to
> > update
> > > > cleared_ in the Texture object. 
> > > > 
> > > > In the failed case invalidate.whole.unbind_read_stencil, only
> > > STENCIL_ATTACHMENT
> > > > of the framebuffer should be invalidated and cleared, and information in
> > > > DEPTH_ATTACHMENT should be kept. But in current implementation, when you
> > > intend
> > > > to mark STENCIL_ATTACHMENT, you can only call the function
> SetLevelCleared()
> > > to
> > > > mark the Texture as a whole by updating the variable cleared_. This can
be
> > > > proved after you call framebuffer->MarkAttachmentAsCleared(...,
> > > STENCIL_BUFFER),
> > > > you can call framebuffer -> HasUnclearedAttachment(DEPTH_ATTACHMENT) and
> > > > framebuffer->HasUnclearedAttachment(STENCIL_ATTACHMENT), and the return
> > values
> > > > will be both true because the variable cleared_ in the Texture object is
> > true.
> > > > 
> > > > It is not enough just to fix ClearUnclearedAttachment(). You can see
> > > function()
> > > > DoDrawArrays() or DoDrawElements() in gles2_cmd_decoder.cc. Before
really
> > > > calling glDrawElements() this function will call
ClearUnclearedTextures()
> at
> > > > first to make sure there is no uncertain values in all textures.
> > > > ClearUnclearedTextures() checks all the textures and call
> > > > texture_manager()->ClearRenderableLevels(), which will finally call the
> > > function
> > > > ClearLevel() in gles2_cmd_decoder.cc. In the previous implementation,
the
> > > > function ClearLevel() will just clear every part of the texture by
> > > > glClear(GL_DEPTH_BUTTER_BIT | GL_STENCIL_BUFFER_BIT). In the situation
> > before,
> > > > as the texture has been marked as uncleared, ClearLevel() is called to
> clear
> > > the
> > > > texture, so the depth part, which should not be cleared, is cleared too.
> > > > 
> > > > So I think the root cause of the bug is that in the previous
> implementation
> > > > Textures with format DEPTH_STENCIL fail to describe the state when only
a
> > part
> > > > of the texture is uncleared, and another part is cleared, so I am trying
> to
> > > fix
> > > > this issue.
> > > 
> > > Thanks for the detailed explanation. I understand now.  So there are only
> two
> > > situations that we might have an uncleared image. 1) it's allocated, but
not
> > > initialized (texImage with null data, renderbuffer) 2) we invalidate it
> > > 
> > > So for 1), we will never clear depth without clearing stencil (or vice
> versa),
> > > so no need to worry about it.
> > > 
> > > for 2), yes, the current implementation is buggy and I still think avoid
> > > InvalidateFramebuffer to the lower driver when we have DEPTH_STENCIL image
> and
> > > we only invalidate depth or stencil bit.
> > > 
> > > Please let me know if you think this might work.
> > 
> > For 2), I think the driver can do the right thing to deal with DEPTH_STENCIL
> > image. You can prove that by commenting the code that marks the attachment
as
> > cleared in invalidateFramebufferImpl(), tests in fboinvalidate can pass, but
> > when you try to draw the invalidated attachment, you can see there is
> uncertain
> > information in the texture. 
> > I fully agree with you and I think we can use glClears to implement
> > invalidateFramebuffer() instead of calling drivers. Maybe we can also use
this
> > way to implement invalidateSubFramebuffer() which is still an empty function
> > now. 
> > 
> > Thanks for your advice. I will try it in this way.
> 
> I don't think we should do clear. It's actually go against the idea of
> InvalidateFramebuffer, which allows drivers to optimize.  It's OK for previous
> contents from the same context to stay, no need to clear them.  We only
> concerned about undefiend behavior, where potentially some vram from other
> contexts could sneak in and leak sensitive information.  I think just leaving
> the above InvalidateFramebuffer case and InvalidateSubFramebuffer as no-op is
> acceptable.  It is also spec compliant, because Spec says undefined contents,
> and leaving image unchanged is one of the undefined behavior.

I agree with Mo's assessment that Chrome should just make the
InvalidateFramebuffer call a no-op if the renderbuffer attachment is
DEPTH_STENCIL and the invalidate call would invalidate only one, but not both,
of those attachments. This is legal according to both the ES 3.0 specification
as well as the WebGL 2.0 specification, which has stricter language:
https://www.khronos.org/registry/webgl/specs/latest/2.0/#5.29 .

We can advise developers that if they want to use InvalidateFramebuffer and get
best performance, they should invalidate both DEPTH and STENCIL attachments at
the same time.

We've seen performance problems in the past on some GPUs in clearing just the
DEPTH or STENCIL portion of a packed DEPTH_STENCIL framebuffer attachment. See
http://crbug.com/584807 . I assume the same performance issues would be present
in real drivers when invalidating one, but not both, of these attachments.

Ken Russell (switch to Gerrit)

On 2016/07/15 02:00:29, Jiawei wrote: > But besides fboinvalidate case, I still think it unacceptable ...

4 years, 5 months ago (2016-07-15 02:04:03 UTC) #20

yunchao

On 2016/07/15 02:02:00, Ken Russell wrote: > On 2016/07/15 01:51:43, Zhenyao Mo wrote: > > ...

4 years, 5 months ago (2016-07-15 02:07:02 UTC) #21

On 2016/07/15 02:02:00, Ken Russell wrote:
> On 2016/07/15 01:51:43, Zhenyao Mo wrote:
> > On 2016/07/15 01:45:23, Jiawei wrote:
> > > On 2016/07/15 01:26:25, Zhenyao Mo wrote:
> > > > On 2016/07/15 01:20:26, Jiawei wrote:
> > > > > On 2016/07/14 22:48:14, Zhenyao Mo wrote:
> > > > > > First, let me clarify this is a bug when we initialize uninitialized
> > > > > > DEPTH_STENCIL resources?
> > > > > > 
> > > > > > In a high level (without looking too much details into the CL), what
> are
> > > the
> > > > > > situations that we ended up clear DEPTH without STENCIL or vice
versa?
> > On
> > > > top
> > > > > of
> > > > > > my head, it is only the cases where you attach a DEPTH_STENCIL to
only
> > > > > > DEPTH_ATTACHMENT or STENCIL_ATTACHMENT, but not both.  Is that the
> case?
> > 
> > > If
> > > > > > yes, then we simply need to fix that in ClearUnclearedAttachment()
> > > function,
> > > > > > which is much less intrusive.
> > > > > > 
> > > > > > But if I miss the point, please help me understand.
> > > > > 
> > > > > OK. Here is all my investigation on this issue. I hope it can make
some
> > help
> > > > to
> > > > > you.
> > > > > 
> > > > > Let me use texture as an example, and the situation of renderbuffer is
> > same.
> > > > The
> > > > > bug I find is that in the previous implementation, we can only mark a
> > > texture
> > > > > cleared or uncleared as a whole, as there is only one variable
cleared_
> in
> > > the
> > > > > texture object.
> > > > > 
> > > > > You can pay attention on the function InvalidateFramebufferImpl() in
> > > > > gles2_cmd_decoder.cc, which is the implementation of WebGL API
> > > > > invalidateFramebuffer(). This function calls
> > > > > framebuffer->MarkAttachmentAsCleared(), which is intended to mark the
> > > specific
> > > > > attachment of the framebuffer cleared or uncleared. 
> > > > > 
> > > > > The class TextureAttachment represents a specific attachment of a
> > > framebuffer,
> > > > > and it wraps the texture attached to the framebuffer. There is a
> > > pointer-like
> > > > > variable called texture_ref which points to the texture. The texture
> with
> > > > format
> > > > > DEPTH_STENCIL should be attached to both DEPTH_ATTACHMENT and
> > > > STENCIL_ATTACHMENT
> > > > > in the same framebuffer when you use textures with this format as
depth
> > and
> > > > > stencil attachments, so texture_refs in both depth TextureAttachment
and
> > > > stencil
> > > > > TextureAttachment point to the same texture with format DEPTH_STENCIL.
> You
> > > can
> > > > > verify this fact by checking the address of texture_ref->texture() in
> > these
> > > > two
> > > > > TextureAttachment objects. The structure of the framebuffer in this
> > > situation
> > > > is
> > > > > as follows:
> > > > > 
> > > > > DEPTH_ATTACHMENT                   STENCIL_ATTACHMENT
> > > > >        |                                   |
> > > > > TextureAttachment                  TextureAttachment
> > > > >         \                                 /
> > > > >          \                               /
> > > > >           Texture in format DEPTH_STENCIL
> > > > > 
> > > > > When framebuffer->MarkAttachmentAsCleared() is called, function
> > SetCleared()
> > > > in
> > > > > the specific TextureAttachment object is called, and finally function
> > > > > SetLevelCleared() in the Texture object is called by TextureManager to
> > > update
> > > > > cleared_ in the Texture object. 
> > > > > 
> > > > > In the failed case invalidate.whole.unbind_read_stencil, only
> > > > STENCIL_ATTACHMENT
> > > > > of the framebuffer should be invalidated and cleared, and information
in
> > > > > DEPTH_ATTACHMENT should be kept. But in current implementation, when
you
> > > > intend
> > > > > to mark STENCIL_ATTACHMENT, you can only call the function
> > SetLevelCleared()
> > > > to
> > > > > mark the Texture as a whole by updating the variable cleared_. This
can
> be
> > > > > proved after you call framebuffer->MarkAttachmentAsCleared(...,
> > > > STENCIL_BUFFER),
> > > > > you can call framebuffer -> HasUnclearedAttachment(DEPTH_ATTACHMENT)
and
> > > > > framebuffer->HasUnclearedAttachment(STENCIL_ATTACHMENT), and the
return
> > > values
> > > > > will be both true because the variable cleared_ in the Texture object
is
> > > true.
> > > > > 
> > > > > It is not enough just to fix ClearUnclearedAttachment(). You can see
> > > > function()
> > > > > DoDrawArrays() or DoDrawElements() in gles2_cmd_decoder.cc. Before
> really
> > > > > calling glDrawElements() this function will call
> ClearUnclearedTextures()
> > at
> > > > > first to make sure there is no uncertain values in all textures.
> > > > > ClearUnclearedTextures() checks all the textures and call
> > > > > texture_manager()->ClearRenderableLevels(), which will finally call
the
> > > > function
> > > > > ClearLevel() in gles2_cmd_decoder.cc. In the previous implementation,
> the
> > > > > function ClearLevel() will just clear every part of the texture by
> > > > > glClear(GL_DEPTH_BUTTER_BIT | GL_STENCIL_BUFFER_BIT). In the situation
> > > before,
> > > > > as the texture has been marked as uncleared, ClearLevel() is called to
> > clear
> > > > the
> > > > > texture, so the depth part, which should not be cleared, is cleared
too.
> > > > > 
> > > > > So I think the root cause of the bug is that in the previous
> > implementation
> > > > > Textures with format DEPTH_STENCIL fail to describe the state when
only
> a
> > > part
> > > > > of the texture is uncleared, and another part is cleared, so I am
trying
> > to
> > > > fix
> > > > > this issue.
> > > > 
> > > > Thanks for the detailed explanation. I understand now.  So there are
only
> > two
> > > > situations that we might have an uncleared image. 1) it's allocated, but
> not
> > > > initialized (texImage with null data, renderbuffer) 2) we invalidate it
> > > > 
> > > > So for 1), we will never clear depth without clearing stencil (or vice
> > versa),
> > > > so no need to worry about it.
> > > > 
> > > > for 2), yes, the current implementation is buggy and I still think avoid
> > > > InvalidateFramebuffer to the lower driver when we have DEPTH_STENCIL
image
> > and
> > > > we only invalidate depth or stencil bit.
> > > > 
> > > > Please let me know if you think this might work.
> > > 
> > > For 2), I think the driver can do the right thing to deal with
DEPTH_STENCIL
> > > image. You can prove that by commenting the code that marks the attachment
> as
> > > cleared in invalidateFramebufferImpl(), tests in fboinvalidate can pass,
but
> > > when you try to draw the invalidated attachment, you can see there is
> > uncertain
> > > information in the texture. 
> > > I fully agree with you and I think we can use glClears to implement
> > > invalidateFramebuffer() instead of calling drivers. Maybe we can also use
> this
> > > way to implement invalidateSubFramebuffer() which is still an empty
function
> > > now. 
> > > 
> > > Thanks for your advice. I will try it in this way.
> > 
> > I don't think we should do clear. It's actually go against the idea of
> > InvalidateFramebuffer, which allows drivers to optimize.  It's OK for
previous
> > contents from the same context to stay, no need to clear them.  We only
> > concerned about undefiend behavior, where potentially some vram from other
> > contexts could sneak in and leak sensitive information.  I think just
leaving
> > the above InvalidateFramebuffer case and InvalidateSubFramebuffer as no-op
is
> > acceptable.  It is also spec compliant, because Spec says undefined
contents,
> > and leaving image unchanged is one of the undefined behavior.
> 
> I agree with Mo's assessment that Chrome should just make the
> InvalidateFramebuffer call a no-op if the renderbuffer attachment is
> DEPTH_STENCIL and the invalidate call would invalidate only one, but not both,
> of those attachments. This is legal according to both the ES 3.0 specification
> as well as the WebGL 2.0 specification, which has stricter language:
> https://www.khronos.org/registry/webgl/specs/latest/2.0/#5.29 .
> 
> We can advise developers that if they want to use InvalidateFramebuffer and
get
> best performance, they should invalidate both DEPTH and STENCIL attachments at
> the same time.
> 
> We've seen performance problems in the past on some GPUs in clearing just the
> DEPTH or STENCIL portion of a packed DEPTH_STENCIL framebuffer attachment. See
> http://crbug.com/584807 . I assume the same performance issues would be
present
> in real drivers when invalidating one, but not both, of these attachments.

Jiawei, let's have a deeper offline investigation and discussion about this CL,
in order to have a good understand about this issue.

Zhenyao Mo

On 2016/07/15 02:04:03, Ken Russell wrote: > On 2016/07/15 02:00:29, Jiawei wrote: > > But ...

4 years, 5 months ago (2016-07-15 02:15:12 UTC) #22

Jiawei

The CQ bit was checked by jiawei.shao@intel.com to run a CQ dry run

4 years, 5 months ago (2016-07-15 02:30:12 UTC) #23

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-15 02:30:24 UTC) #24

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-15 02:30:26 UTC) #25

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-15 02:30:28 UTC) #26

commit-bot: I haz the power

Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started ...

4 years, 5 months ago (2016-07-15 02:30:28 UTC) #27

Jiawei

Description was changed from ========== Add depth_cleared_ and stencil_cleared_ for class texture and renderbuffer. Textures ...

4 years, 5 months ago (2016-07-18 00:21:34 UTC) #28

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 00:23:09 UTC) #29

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-18 00:26:26 UTC) #35

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-18 00:26:28 UTC) #36

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-18 00:26:30 UTC) #37

commit-bot: I haz the power

No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...

4 years, 5 months ago (2016-07-18 00:26:31 UTC) #38

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 01:09:17 UTC) #40

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 01:09:40 UTC) #41

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 01:09:55 UTC) #42

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 01:10:09 UTC) #43

qiankun

Hi Ken & Mo, Could you help to review it now? It only deals with ...

4 years, 5 months ago (2016-07-18 04:50:09 UTC) #44

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 05:04:28 UTC) #45

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 05:05:09 UTC) #46

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 05:05:21 UTC) #47

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 05:05:48 UTC) #48

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-18 05:07:59 UTC) #49

Zhenyao Mo

On 2016/07/18 04:50:09, qiankun wrote: > Hi Ken & Mo, > Could you help to ...

4 years, 5 months ago (2016-07-18 05:48:07 UTC) #50

qiankun

On 2016/07/18 05:48:07, Zhenyao Mo wrote: > On 2016/07/18 04:50:09, qiankun wrote: > > Hi ...

4 years, 5 months ago (2016-07-18 07:33:58 UTC) #51

Ken Russell (switch to Gerrit)

https://codereview.chromium.org/2149523002/diff/80001/gpu/command_buffer/service/framebuffer_manager.cc File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2149523002/diff/80001/gpu/command_buffer/service/framebuffer_manager.cc#newcode538 gpu/command_buffer/service/framebuffer_manager.cc:538: // TODO: when the texture or the renderbuffer in ...

4 years, 5 months ago (2016-07-18 18:34:20 UTC) #52

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-19 02:19:55 UTC) #54

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-19 02:19:58 UTC) #55

Zhenyao Mo

Sorry I unchecked commit button. Please address kbr's comments first.

4 years, 5 months ago (2016-07-19 02:58:19 UTC) #57

Jiawei

On 2016/07/19 02:58:19, Zhenyao Mo wrote: > Sorry I unchecked commit button. Please address kbr's ...

4 years, 5 months ago (2016-07-19 03:08:13 UTC) #58

Jiawei

On 2016/07/18 18:34:20, Ken Russell wrote: > https://codereview.chromium.org/2149523002/diff/80001/gpu/command_buffer/service/framebuffer_manager.cc > File gpu/command_buffer/service/framebuffer_manager.cc (right): > > https://codereview.chromium.org/2149523002/diff/80001/gpu/command_buffer/service/framebuffer_manager.cc#newcode538 ...

4 years, 5 months ago (2016-07-19 11:46:16 UTC) #59

Zhenyao Mo

On 2016/07/19 11:46:16, Jiawei wrote: > On 2016/07/18 18:34:20, Ken Russell wrote: > > > ...

4 years, 5 months ago (2016-07-19 16:35:48 UTC) #60

On 2016/07/19 11:46:16, Jiawei wrote:
> On 2016/07/18 18:34:20, Ken Russell wrote:
> >
>
https://codereview.chromium.org/2149523002/diff/80001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/framebuffer_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2149523002/diff/80001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/framebuffer_manager.cc:538: // TODO: when the
> texture
> > or the renderbuffer in format DEPTH_STENCIL, mark
> > This TODO needs an owner; see
> > https://google.github.io/styleguide/cppguide.html#TODO_Comments . Please
> either
> > add yourself or zmo. Thanks.
> > 
> >
>
https://codereview.chromium.org/2149523002/diff/80001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/2149523002/diff/80001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:5773: if (attachments[i] ==
> > GL_DEPTH_STENCIL_ATTACHMENT) {
> > This looks wrong. It's referencing the attachments array, but count is the
> > length of the validated_attachments array, which might be less.
> > 
> > A unit test is needed for this change, in framebuffer_manager_unittest.cc.
> 
> I found the unit test for InvalidateFramebuffer is defined in
> gles2_cmd_decoder_unittest_framebuffer.cc. Do you mean I should just add tests
> to test the added function in framebuffer_manager?

Thanks Ken for finding this bug in this CL.  Basically your CL does not fix the
WebGL failing test at all.  At least you should verify the CL fixes the bug you
try to fix.

A unittest should be added to gles2_cmd_decoder_unittest_framebuffer.cc where
you should create a framebuffer, a renderbuffer with DEPTH_STENCIL format, and
set it as cleared.  Then trigger an invalidateFramebuffer(DEPTH) call, and make
sure 1) the GL's invalidateFramebuffer is not called 2) the renderbuffer remains
cleared().  If you need help writing this test, just fix the bug in this CL
first, then I can help with writing the unittest.

Zhenyao Mo

jiawei, per offline discussion with kbr, please fix the bug and get the conformance test ...

4 years, 5 months ago (2016-07-19 20:48:19 UTC) #61

Jiawei

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-20 01:23:52 UTC) #62

Jiawei

On 2016/07/19 20:48:19, Zhenyao Mo wrote: > jiawei, per offline discussion with kbr, please fix ...

4 years, 5 months ago (2016-07-20 08:08:32 UTC) #63

qiankun

https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode7040 gpu/command_buffer/service/gles2_cmd_decoder.cc:7040: framebuffer->AttachRenderbuffer(attachment, renderbuffer); If attachment is GL_DEPTH_STENCIL_ATTACHMENT, framebuffer will attach ...

4 years, 5 months ago (2016-07-20 18:54:57 UTC) #65

Zhenyao Mo

https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode7040 gpu/command_buffer/service/gles2_cmd_decoder.cc:7040: framebuffer->AttachRenderbuffer(attachment, renderbuffer); On 2016/07/20 18:54:57, qiankun wrote: > If ...

4 years, 5 months ago (2016-07-20 19:08:20 UTC) #66

Zhenyao Mo

https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode7040 gpu/command_buffer/service/gles2_cmd_decoder.cc:7040: framebuffer->AttachRenderbuffer(attachment, renderbuffer); On 2016/07/20 19:08:20, Zhenyao Mo wrote: > ...

4 years, 5 months ago (2016-07-20 19:09:51 UTC) #67

qiankun

On 2016/07/20 19:09:51, Zhenyao Mo wrote: > https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode7040 ...

4 years, 5 months ago (2016-07-20 19:13:43 UTC) #68

Zhenyao Mo

https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode7040 gpu/command_buffer/service/gles2_cmd_decoder.cc:7040: framebuffer->AttachRenderbuffer(attachment, renderbuffer); On 2016/07/20 19:09:51, Zhenyao Mo wrote: > ...

4 years, 5 months ago (2016-07-20 19:23:54 UTC) #69

Ken Russell (switch to Gerrit)

Sorry for the delay re-reviewing -- LGTM. https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/framebuffer_manager.cc File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2149523002/diff/100001/gpu/command_buffer/service/framebuffer_manager.cc#newcode574 gpu/command_buffer/service/framebuffer_manager.cc:574: // TODO(Jiawei): ...

4 years, 5 months ago (2016-07-20 23:36:06 UTC) #70

Jiawei

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, zmo@chromium.org Link to ...

4 years, 5 months ago (2016-07-21 08:34:38 UTC) #72

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-21 08:34:50 UTC) #73

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-21 08:34:53 UTC) #74

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-21 09:01:35 UTC) #77

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-21 09:01:39 UTC) #78

qiankun

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-21 09:27:29 UTC) #79

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-21 09:31:32 UTC) #80

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-21 11:23:30 UTC) #83

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 5 months ago (2016-07-21 11:23:34 UTC) #84

qiankun

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-21 11:29:47 UTC) #85

commit-bot: I haz the power

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

4 years, 5 months ago (2016-07-21 11:30:42 UTC) #88

commit-bot: I haz the power

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-21 12:02:30 UTC) #89

commit-bot: I haz the power

Description was changed from ========== Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment Due to potential performance ...

4 years, 5 months ago (2016-07-21 12:04:44 UTC) #91

commit-bot: I haz the power

4 years, 5 months ago (2016-07-21 12:04:46 UTC) #92

Message was sent while issue was closed.

Patchset 7 (id:??) landed as
https://crrev.com/ce7e217303925cfec9b453ff745e9ebb5f120d95
Cr-Commit-Position: refs/heads/master@{#406821}

Issue 2149523002: Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment (Closed)

Description

Patch Set 1 #

Patch Set 2 : Add depth_cleared_ and stencil_cleared_ for class texture and renderbuffer. #

Patch Set 3 : Add depth_cleared_ and stencil_cleared_ for class texture and renderbuffer. #

Patch Set 4 : Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment #

Patch Set 5 : Make invalidateFramebuffer no-op for DEPTH_STENCIL attachment #

Patch Set 6 : Fix bugs found by Ken #

Patch Set 7 : Use full email address in TODO #

Messages