Please correct me if I misunderstood clear_state_dirty.

see the small patch at https://codereview.chromium.org/2613793003/.

I think your understanding is correct. We set clear_state_dirty_ more often than necessary. Likely RestoreClearState isn't a heavy operation, so we never notice this. That said, if you can clean this up, it's definitely good for code health and readability.

Sorry I spoke before I read the code carefully.

If you look at GLES2DecoderImpl::ApplyDirtyState(), where you enable the masks depending on if the fbo contains the corresponding attachments. i.e., if we don't have depth image attached, then we don't enable depth even if the user called DepthMask(true).

Therefore, the fbo change change does affect the clear_state_dirty_.

For example, if you set depth mask to true, but no depth image is attached.  Now you draw, ApplyDirtyState() will leave depth mask to be false.  Now you attach a depth image, and draw again, if clear_state_dirty_ isn't set to true, then ApplyDirtyState will do nothing and depth mask remains false, which is a mistake.

Please close this bug if you agree with my analysis.

Thanks for your explanation, @zmo. But I have different opinion. For the situation you said, the question is that why we need to enable/disable depth/stencil test according to whether we have depth/stencil attachment in fbo? 

Per my understanding, all of the states (masks, enable or disable depth/stencil test) are per context, not per fbo. So I think we should not define framebuffer_state_.clear_state_dirty in framebuffer_manager.cc, but to define state_.clear_state_dirty in context_state.cc 

I wonder that do we have a driver bug if we enable depth/stencil test but there is no depth/stencil attachment? If that is true, maybe we should do this in a workaround for specific driver, instead of enabling/disabling depth/stencil test in ApplyDirtyState. I indeed see that we frequently set clear_state_dirty and call ApplyDirtyState, although it doesn't call into driver every time because of the cache mechanism.

BTW, the patch at https://codereview.chromium.org/2613793003/ remove the code snippet to set clear_state_dirty when the image pixels are changed by TexImage2D, TexStorage2D, CompressedTexImage2D. I think it is not necessary. 

Even in the current code design, if an image is attached/unattached to fbo, clear_state_dirty have been set in framebuffer{Textuer2D|TextureLayer} already. It is not necessary to set clear_state_dirty when we update the image pixels by TexImage2D...

Note that DecoderFramebufferState is per FramebufferManager, not per Framebuffer, so it is per context.

As for why we enable/disable depth/stencil, the command buffer's basic design philosophy is we don't trust the drivers so we always try to be on the cautious side if it's not affecting perf. I don't think we need to revisit this and put it behind a driver bug workaround.

Ah, you are correct, @zmo. clear_state_dirty is per context in current code. 

However, I don't think it is necessary to set or unset clear_state_dirty according to whether we have color/depth/stencil buffer in current fbo. If we remove these unnecessary code, all WebGL CTS can pass, see the change at: https://codereview.chromium.org/2612523003/ (The failures are in gpu_unittests, because some gl operations will not be called any more when clear_dirty_state is removed). 

In the current code, there are many call sites to ApplyDirtyState() and gl operations to enable/disable masks (I have added some printf to check about this in my local machine). If we remove the set/unset code, almost all call sites to ApplyDirtyState() and gl operations to enable/disable masks are gone for most conformance tests. So the code cleanup can improve performance.

The other reason that I want to remove most of the clear_state_dirty: I need a flag like fboimage_state_dirty to detect feedback loop when fbo images are changed by bindFramebuffer/deleteFramebuffer/framebufferTexture2D/framebufferTextureLayer/drawBuffers/framebufferRenderbuffer/etc. You see, This flag will be set/unset in functions in which we set clear_state_dirty too. If we have 2 flags, it seems to be confusing... 

So I think this code cleanup about clear_state_dirty may be reasonable, WDYT?

I am not worried about 2 flags being confusing, but this extra optimization with clear_state_dirty does feel adding more burden than benefits.  If we won't trigger any driver bugs, then I am OK with removing it when fbo/texture/renderbuffer change.

OK, I will have a try in these two days.

Sorry. It is a long time to revisit this issue.

I tried to remove clear_state_dirty in command buffer to remove redundant gl call sites. It indeed works for all WebGL CTS. All webgl cts can pass on trybots.

However, the gpu_unittests have lots of expectations to call these gl operations. If the gl call sites to driver are removed, the expectations will not be satisfied, making the corresponding gpu_unittest failed. And the code in gpu_unittests related to the expectation functions scattered here and there. It is rather time-consuming to remove all these expectations in gpu_unittests. So I would not fix this issue.

Please re-open this one if anyone want to fix this issue.