Avoid to resolve CMAA for read and draw without content changed or with content committed.

third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp

 /*
  * Copyright (c) 2010, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 966 matching lines @@
   {
     ScopedStateRestorer scoped_state_restorer(this);
     ResolveIfNeeded();
   }
   gl_->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
 }
 
 void DrawingBuffer::ResolveMultisampleFramebufferInternal() {
   DCHECK(state_restorer_);
   state_restorer_->SetFramebufferBindingDirty();
-  if (WantExplicitResolve() && !contents_change_resolved_) {
+  if (WantExplicitResolve()) {
     state_restorer_->SetClearStateDirty();
     gl_->BindFramebuffer(GL_READ_FRAMEBUFFER_ANGLE, multisample_fbo_);
     gl_->BindFramebuffer(GL_DRAW_FRAMEBUFFER_ANGLE, fbo_);
     gl_->Disable(GL_SCISSOR_TEST);
 
     int width = size_.Width();
     int height = size_.Height();
     // Use NEAREST, because there is no scale performed during the blit.
     GLuint filter = GL_NEAREST;
 
@@ skipping 12 matching lines @@
       gl_->Clear(GL_COLOR_BUFFER_BIT);
     }
   }
 
   gl_->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
   if (anti_aliasing_mode_ == kScreenSpaceAntialiasing)
     gl_->ApplyScreenSpaceAntialiasingCHROMIUM();
 }
 
 void DrawingBuffer::ResolveIfNeeded() {
-  if (anti_aliasing_mode_ != kNone)
+  if (anti_aliasing_mode_ != kNone && !contents_change_resolved_)

[Zhenyao Mo, 2017/04/13 17:36:06]

This is a behavioral change.  Because before even if contents_change_resolved_
is true, we still bind fbo_ (line $1019), but now we don't.

Is it OK? Please do a bit careful study and provide an answer (with explanation)
here.

[xinghua.cao, 2017/04/26 10:46:37]

Zhenyao, I had made a more carefully investigation on this issue. List my
comment as below,
I think DrawingBuffer::ResolveMultisampleFramebufferInternal is used to only
resolve the multisampled buffer into back_color_buffer_ texture.
ResolveIfNeeded function is called by
DrawingBuffer::PrepareTextureMailboxInternal,
DrawingBuffer::CopyToPlatformTexture and
DrawingBuffer::ResolveAndBindForReadAndDraw.
DrawingBuffer::PrepareTextureMailboxInternal and
DrawingBuffer::CopyToPlatformTexture only operate back_color_buffer_ texture, do
not operate fbo, and do not require bind to fbo_.
DrawingBuffer::ResolveAndBindForReadAndDraw had already binded fbo_ to
GL_FRAMEBUFFER target after resolving multisample, because it need to read and
draw fbo_.
In summary, I think binding fbo_ is only needed if we want to operate the fbo_.
if we only want to operate attached texture, does not need bind fbo_, and I
think only read and draw need to bind fbo_ to GL_FRAMEBUFFER after resolve
multisample. And if one function wants operate fbo_, it should explicitly bind
fbo_ in its body, like DrawingBuffer::ResolveAndBindForReadAndDraw. Is my
understanding right?
I think that binding fbo_ (line $1014) is only useful for
ApplyScreenSpaceAntialiasingCHROMIUM (CMAA operation, anti_aliasing_mode is
kScreenSpaceAntialiasing) if GL_FRAMEBUFFER target is currently binding to zero,
if not, it is already binding on fbo_.

By the way, I have another question. Why should bind to fbo_ after every resolve
multisample? If it was originally binding to multisample_fbo_, after resolve
multsample, it is binding to fbo_. Is that right?

[Ken Russell (switch to Gerrit), 2017/04/28 00:10:00]

This sounds correct. One note: DrawingBuffer::PrepareTextureMailboxInternal
calls ResolveIfNeeded and immediately calls FinishPrepareTextureMailboxGpu.
FinishPrepareTextureMailboxGpu calls AttachColorBufferToReadFramebuffer -- which
sounds like it's relying on the currently bound framebuffer -- but in fact it
binds fbo_ itself.
I agree. It looks like that could be done only if we're going to call
ApplyScreenSpaceAntialiasingCHROMIUM.
I don't see a good reason for this and think this could be changed.

[dshwang, 2017/04/28 02:08:29]

I think so.
I agree. as ResolveIfNeeded() is private method, public caller must take care of
fbo state.

[dshwang, 2017/04/28 02:48:42]

ResolveMultisampleFramebufferInternal() set fbo dirty to state_restorer_, so
it's ok to bind any FBO in this moment.

     ResolveMultisampleFramebufferInternal();
   contents_change_resolved_ = true;
 }
 
 void DrawingBuffer::RestoreFramebufferBindings() {
   client_->DrawingBufferClientRestoreFramebufferBinding();
 }
 
 void DrawingBuffer::RestoreAllState() {
   client_->DrawingBufferClientRestoreScissorTest();
@@ skipping 29 matching lines @@
   data_size *= height;
   if (!data_size.IsValid())
     return false;
 
   WTF::ArrayBufferContents pixels(width * height, 4,
                                   WTF::ArrayBufferContents::kNotShared,
                                   WTF::ArrayBufferContents::kDontInitialize);
 
   GLuint fbo = 0;
   state_restorer_->SetFramebufferBindingDirty();
   if (source_buffer == kFrontBuffer && front_color_buffer_) {

[xinghua.cao, 2017/04/28 02:40:08]

Hi, all, another question here.
I think if context is currently binding to default fbo, GL_FRAMEBUFFER target
must be binded either fbo_ or multisample_fbo_, and never binded to zero.
But here if this condition is true, will create a new fbo object, when calling
gl_->DeleteFramebuffers(1, &fbo) below, GL_FRAMEBUFFER target is implicitly
binded to zero. Binding status had been changed here.
Is it right here?

[dshwang, 2017/04/28 02:48:42]

public methods are guard by |state_restorer_| and line1069 checks fbo is dirty.

[xinghua.cao, 2017/04/28 02:56:02]

Yes, here is no problem. Thank you @dshwang.

     gl_->GenFramebuffers(1, &fbo);
     gl_->BindFramebuffer(GL_FRAMEBUFFER, fbo);
     gl_->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
                               front_color_buffer_->parameters.target,
                               front_color_buffer_->texture_id, 0);
   } else {
     gl_->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
   }
 
   ReadBackFramebuffer(static_cast<unsigned char*>(pixels.Data()), width, height,
@@ skipping 220 matching lines @@
   if (pixel_unpack_buffer_binding_dirty_)
     client->DrawingBufferClientRestorePixelUnpackBufferBinding();
 }
 
 bool DrawingBuffer::ShouldUseChromiumImage() {
   return RuntimeEnabledFeatures::webGLImageChromiumEnabled() &&
          chromium_image_usage_ == kAllowChromiumImage;
 }
 
 }  // namespace blink