Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

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

Index: third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
index 070cb6d56d319404d205b392a0750df0e855044a..8d4bf5a1df9e6489ae20f5d75e35990e78b26b1d 100644
--- a/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
+++ b/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
@@ -299,6 +299,7 @@ bool DrawingBuffer::PrepareTextureMailboxInternal(
 bool DrawingBuffer::FinishPrepareTextureMailboxSoftware(
     cc::TextureMailbox* out_mailbox,
     std::unique_ptr<cc::SingleReleaseCallback>* out_release_callback) {
+  DCHECK(state_restorer_);
   std::unique_ptr<cc::SharedBitmap> bitmap = CreateOrRecycleBitmap();
   if (!bitmap)
     return false;
@@ -311,6 +312,8 @@ bool DrawingBuffer::FinishPrepareTextureMailboxSoftware(
     WebGLImageConversion::AlphaOp op =
         need_premultiply ? WebGLImageConversion::kAlphaDoPremultiply
                          : WebGLImageConversion::kAlphaDoNothing;

[xinghua.cao, 2017/05/24 11:24:39]

Here may be binding framebuffer on multisample_fbo_, GL_INVALID_OPERATION
happens when calling readPixels in ReadBackFramebuffer function. So we need to
be sure that currently bind framebuffer on fbo_, and restore binding status
after PrepareTextureMailbox

+    state_restorer_->SetFramebufferBindingDirty();
+    gl_->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
     ReadBackFramebuffer(pixels, Size().Width(), Size().Height(), kReadbackSkia,

[Zhenyao Mo, 2017/05/24 20:31:01]

I think BindFramebuffer(_, fbo_) should be part of ReadBackFramebuffer().

[Ken Russell (switch to Gerrit), 2017/05/24 20:40:04]

Looking at the code, I'm not sure it can be.
DrawingBuffer::PaintRenderingResultsToImageData has the option of reading back
the front buffer, in which case a temporary FBO is allocated and bound.

[Zhenyao Mo, 2017/05/24 20:53:30]

kbr is right. Sorry about the noise. The code is fine as is.  Please address
capn's test change request.

[xinghua.cao, 2017/05/25 10:43:17]

Hi, all, I am confused how to design the test case for this patch in
DrawingBufferSoftwareRenderingTest.cpp. I do not know what is expected result
for this case. Does we need to verify the framebuffer object currently is being
binded? Or the TextureMailbox object return by PrepareTextureMailbox? But I
think they cannot verify validity of this patch.
Do you have some good idea? Thank you. 

[Zhenyao Mo, 2017/05/25 18:03:00]

May check gl errors after PrepareTextureMailbox() call in the test would be
enough?

Or you can check current fbo binding before and after the
PrepareTextureMailbox() call and make sure they don't change?

[xinghua.cao, 2017/05/26 15:26:23]

I think it is more suitable to check the fbo binding.
And it is difficult to find rules to report GL_INVALID_OPERATION in readPixels,
which should be overrided DrawingBufferTestHelpers.h.

                         op);
   }