[Command buffer] Feedback loop detection between texture and framebuffer attachments

gpu/command_buffer/service/gles2_cmd_decoder.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index 8a06bd07861e3907873d3f6aedd8e5ca6c50f31b..598a1248fac732e810e8b6869a75f086ef36f120 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -1322,6 +1322,11 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   // of the draw operation are the same.
   bool CheckDrawingFeedbackLoops();
 
+  // Helper for CheckDrawingFeedbackLoops. Returns true if the attachment is
+  // the same one where it samples from during drawing.
+  bool CheckDrawingFeedbackLoopsHelper(
+    const Framebuffer::Attachment* attachment);
+
   bool SupportsDrawBuffers() const;
 
   // Checks if a draw buffer's format and its corresponding fragment shader
@@ -8646,15 +8651,8 @@ bool GLES2DecoderImpl::CheckCurrentProgramForUniform(
       location);
 }
 
-bool GLES2DecoderImpl::CheckDrawingFeedbackLoops() {
-  Framebuffer* framebuffer = GetFramebufferInfoForTarget(GL_FRAMEBUFFER);
-  if (!framebuffer)
-    return false;
-  const Framebuffer::Attachment* attachment =
-      framebuffer->GetAttachment(GL_COLOR_ATTACHMENT0);
-  if (!attachment)
-    return false;
-
+bool GLES2DecoderImpl::CheckDrawingFeedbackLoopsHelper(
+    const Framebuffer::Attachment* attachment) {
   DCHECK(state_.current_program.get());
   const Program::SamplerIndices& sampler_indices =
       state_.current_program->sampler_indices();
@@ -8676,6 +8674,36 @@ bool GLES2DecoderImpl::CheckDrawingFeedbackLoops() {
   return false;
 }
 
+bool GLES2DecoderImpl::CheckDrawingFeedbackLoops() {
+  if (feature_info_->IsWebGL1OrES2Context()) {

[Zhenyao Mo, 2016/10/31 18:03:46]

We are able to expose MRT in WebGL1 through extension, so you will need to
consider that possibility.

[yunchao, 2016/11/04 15:30:01]

Done.

+    Framebuffer* framebuffer = GetFramebufferInfoForTarget(GL_FRAMEBUFFER);
+    if (!framebuffer)
+      return false;
+    const Framebuffer::Attachment* attachment =
+        framebuffer->GetAttachment(GL_COLOR_ATTACHMENT0);
+    if (!attachment)
+      return false;
+    return CheckDrawingFeedbackLoopsHelper(attachment);
+  } else {
+    Framebuffer* framebuffer = GetFramebufferInfoForTarget(GL_DRAW_FRAMEBUFFER);
+    if (!framebuffer)
+      return false;
+    for (uint32_t ii = 0; ii < group_->max_draw_buffers(); ++ii) {

[piman, 2016/11/02 02:33:57]

Do we need to handle depth attachment vs depth texture too? (and ditto stencil).

[yunchao, 2016/11/04 15:30:01]

I think so.

+      GLenum drawbuffer = static_cast<GLenum>(GL_DRAW_BUFFER0 + ii);
+      if (framebuffer->GetDrawBuffer(drawbuffer) == GL_NONE)
+        continue;
+      GLenum attachment = static_cast<GLenum>(GL_COLOR_ATTACHMENT0 + ii);
+      const Framebuffer::Attachment* draw_buffer =
+          framebuffer->GetAttachment(attachment);
+      if (!draw_buffer)
+        continue;
+      if (CheckDrawingFeedbackLoopsHelper(draw_buffer))

[Zhenyao Mo, 2016/10/31 18:03:46]

We will want to optimize on this.  This is not trivia and we should be able to
avoid doing it per draw call. i.e., we should be able to only do it when related
states changed, including drawBuffers, framebufferTex calls, etc.

[yunchao, 2016/11/01 15:25:22]

The cache info is not easy if we try to optimize on this, we need to cache lots
of infos. In addition, if we cache the info, we still need to either 1) iterate
and calculate the current drawbuffers mask (to compare), iterate the framebuffer
attachments masks, iterate the active texture/sampler binding points masks, or
2) we set dirty states for all these related call sites. The code will be much
more complicated to detect feedback loop. If anyone is dirty, we still need to
do the validation.

For the current code, I think it will early 'continue' for common cases. we
still only have COLOR_ATTACHMENT0 as draw buffer for the majority of draw calls.
Even for MRT, most of the for loop will early 'continue' too in
CheckDrawFeedbackLoopsHelper(). 

WDYT, Zhenyao?

If you still think the optimization can bring in a great improvement against
current implementation, I will have a try. 

[Zhenyao Mo, 2016/11/01 22:55:46]

I think we just need one state, indicating if a feedback loop exists between
drawing dst / sampling src.

Then we could do some rough dirty-bit updating:

1) At program link succeed time, gather which texture binding points are used by
this program, i.e., a set of (active_texture, sampler_type)
2) set dirty flag to the state if the following happens
* drawBuffers()
* framebufferTexture{2|3}D (if the binding point is in set from step 1)
* changing of the current program (useProgram that changed the program,
linkProgram of the currently used program)
* bindTexture (which changed the binding) if the binding point is in set from
step 1)

[yunchao, 2016/11/04 15:30:01]

That's a pretty good suggestion. I will update the code here and find out a good
solution after I go through all undefined behavior in ES3 spec.

+        return true;
+    }
+    return false;
+  }
+}
+
 bool GLES2DecoderImpl::SupportsDrawBuffers() const {
   return feature_info_->IsWebGL1OrES2Context() ?
       feature_info_->feature_flags().ext_draw_buffers : true;