Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

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 281 matching lines @@
     return FinishPrepareTextureMailboxSoftware(out_mailbox,
                                                out_release_callback);
   } else {
     return FinishPrepareTextureMailboxGpu(out_mailbox, out_release_callback);
   }
 }
 
 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;
 
   // Read the framebuffer into |bitmap|.
   {
     unsigned char* pixels = bitmap->pixels();
     DCHECK(pixels);
     bool need_premultiply = want_alpha_channel_ && !premultiplied_alpha_;
     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);
   }
 
   *out_mailbox = cc::TextureMailbox(bitmap.get(), size_);
   out_mailbox->set_color_space(color_space_);
 
   // This holds a ref on the DrawingBuffer that will keep it alive until the
   // mailbox is released (and while the release callback is running). It also
   // owns the SharedBitmap.
   auto func = WTF::Bind(&DrawingBuffer::MailboxReleasedSoftware,
@@ skipping 1055 matching lines @@
   if (pixel_unpack_buffer_binding_dirty_)
     client->DrawingBufferClientRestorePixelUnpackBufferBinding();
 }
 
 bool DrawingBuffer::ShouldUseChromiumImage() {
   return RuntimeEnabledFeatures::webGLImageChromiumEnabled() &&
          chromium_image_usage_ == kAllowChromiumImage;
 }
 
 }  // namespace blink