[Command buffer] Fix the bug when blitting pixels outside read framebuffer

gpu/command_buffer/service/gles2_cmd_decoder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
 
 #include <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <stdio.h>
@@ skipping 7972 matching lines @@
     if ((GetBoundFramebufferDepthFormat(GL_READ_FRAMEBUFFER) !=
          GetBoundFramebufferDepthFormat(GL_DRAW_FRAMEBUFFER)) ||
         (GetBoundFramebufferStencilFormat(GL_READ_FRAMEBUFFER) !=
          GetBoundFramebufferStencilFormat(GL_DRAW_FRAMEBUFFER))) {
       LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name,
                          "src and dst formats differ for depth/stencil");
       return;
     }
   }
 
+  if (workarounds().adjust_src_dst_region_for_blitframebuffer) {
+    gfx::Size read_size = GetBoundReadFramebufferSize();
+    gfx::Rect rect(0, 0, read_size.width(), read_size.height());

[Corentin Wallez, 2016/10/14 14:42:42]

rect isn't a very descriptive name, use "source_bounds" or something?

[yunchao, 2016/10/15 14:34:58]

That's true. I have updated the name accordingly. 

+    GLuint src_x = srcX1 > srcX0 ? srcX0 : srcX1;
+    GLuint src_y = srcY1 > srcY0 ? srcY0 : srcY1;
+    base::CheckedNumeric<GLuint> src_width_temp =
+        srcX1 > srcX0 ? srcX1 - srcX0 : srcX0 - srcX1;

[Zhenyao Mo, 2016/10/14 17:54:15]

I am not sure if this will catch the overlow or not.  The overflow happens
during "-", not in "=".

Correct me if I am wrong, but I think this should be something like

base::CheckedNumeric<GLint> src_width_temp = srcX1; // use int, not uint
src_width_temp -= srcX0;
GLuint src_width = ::abs(src_width_temp.ValueOrDefault(...));

(same for other places)

[yunchao, 2016/10/15 14:34:57]

Yes, you are correct, Zhenyao. The overflow happens during "-" or "+", not "='.
The other places do it in the same way as you suggested. 

+    base::CheckedNumeric<GLuint> src_height_temp =
+        srcY1 > srcY0 ? srcY1 - srcY0 : srcY0 - srcY1;
+    GLuint src_width = src_width_temp.ValueOrDie();

[Corentin Wallez, 2016/10/14 14:42:42]

ValueOrDie just returns the value and does a runtime check that there is not an
error (crashes on error). I think we should check there are not errors above.

[Zhenyao Mo, 2016/10/14 17:54:15]

We don't really have an error case defined if width or height goes beyond int32.

Thinking about this: if we specify (src0, src1) to be (-INT_MAX, INT_MAX),
(dst0, dst1) to be (_INT_MAX, INT_MAX).  If src and dst is 1:1, then it's the
same as (0, max) and (0, max).

I am not sure what's the best thing to do in those scenarios.  Any suggestions?
The easiest is to just always generante INVALID_VALUE if overflow ever happens,
but we will need to document that in WebGL2 spec.

[Zhenyao Mo, 2016/10/14 21:10:32]

Per working group discussion, we are going to generate an error whenever there
is an overflow in internal computation.  No need to update the WebGL2 spec.  I
suggest we generate INVALID_OPERATION instead.

[yunchao, 2016/10/15 14:34:57]

Done.

[yunchao, 2016/10/15 14:34:58]

Done.

+    GLuint src_height = src_height_temp.ValueOrDie();
+    gfx::Rect src_region(src_x, src_y, src_width, src_height);
+    if (!rect.Contains(src_region) && (src_width != 0) && (src_height != 0)) {
+      // If pixels lying outside the read framebuffer, adjust src region
+      // and dst region to appropriate in-bounds regions respectively.
+      rect.Intersect(gfx::Rect(src_x, src_y, src_width, src_height));

[Corentin Wallez, 2016/10/14 14:42:42]

rect.Intersect(src_region)

[yunchao, 2016/10/15 14:34:57]

Yeah... Will update the code here.

+      GLuint src_real_width = rect.width();
+      GLuint src_real_height = rect.height();
+      base::CheckedNumeric<GLuint> xoffset_temp = rect.x() - src_x;
+      base::CheckedNumeric<GLuint> yoffset_temp = rect.y() - src_y;
+      // if X/Y is reversed, use the top/right out-of-bounds region for mapping
+      // to dst region, instead of left/bottom out-of-bounds region for mapping.
+      if (((srcX1 > srcX0) && (dstX1 < dstX0)) ||
+          ((srcX1 < srcX0) && (dstX1 > dstX0))) {
+        xoffset_temp = src_x + src_width - rect.x() - rect.width();
+      }
+      if (((srcY1 > srcY0) && (dstY1 < dstY0)) ||
+          ((srcY1 < srcY0) && (dstY1 > dstY0))) {
+        yoffset_temp = src_y + src_height - rect.y() - rect.height();
+      }
+      GLuint xoffset = xoffset_temp.ValueOrDie();

[Corentin Wallez, 2016/10/14 14:42:42]

Same as above for the CheckedNumerics

[yunchao, 2016/10/15 14:34:58]

Done

+      GLuint yoffset = yoffset_temp.ValueOrDie();
+
+      GLuint dst_x = dstX1 > dstX0 ? dstX0 : dstX1;
+      GLuint dst_y = dstY1 > dstY0 ? dstY0 : dstY1;
+      GLuint dst_width = dstX1 > dstX0 ? dstX1 - dstX0 : dstX0 - dstX1;
+      GLuint dst_height = dstY1 > dstY0 ? dstY1 - dstY0 : dstY0 - dstY1;
+      GLfloat dst_mapping_width =
+          static_cast<GLfloat>(src_real_width) * dst_width / src_width;
+      GLfloat dst_mapping_height =
+          static_cast<GLfloat>(src_real_height) * dst_height / src_height;
+      GLfloat dst_mapping_xoffset =
+          static_cast<GLfloat>(xoffset) * dst_width / src_width;
+      GLfloat dst_mapping_yoffset =
+          static_cast<GLfloat>(yoffset) * dst_height / src_height;
+
+      GLuint dst_mapping_x0 =

[yunchao, 2016/10/14 13:49:05]

Use std::round for both NEAREST and LINEAR filter. 

+          std::round(dst_x + dst_mapping_xoffset);
+      GLuint dst_mapping_y0 =
+          std::round(dst_y + dst_mapping_yoffset);
+
+      GLuint dst_mapping_x1 =
+          std::round(dst_x + dst_mapping_xoffset + dst_mapping_width);
+      GLuint dst_mapping_y1 =
+          std::round(dst_y + dst_mapping_yoffset + dst_mapping_height);
+
+      // adjust the src region and dst region to fit the read framebuffer
+      srcX0 = srcX0 < srcX1 ? rect.x() : rect.x() + rect.width();
+      srcY0 = srcY0 < srcY1 ? rect.y() : rect.y() + rect.height();
+      srcX1 = srcX0 < srcX1 ? rect.x() + rect.width() : rect.x();
+      srcY1 = srcY0 < srcY1 ? rect.y() + rect.height() : rect.y();
+
+      dstX0 = dstX0 < dstX1 ? dst_mapping_x0 : dst_mapping_x1;
+      dstY0 = dstY0 < dstY1 ? dst_mapping_y0 : dst_mapping_y1;
+      dstX1 = dstX0 < dstX1 ? dst_mapping_x1 : dst_mapping_x0;
+      dstY1 = dstY0 < dstY1 ? dst_mapping_y1 : dst_mapping_y0;
+    }
+  }
+
   bool enable_srgb =
       (read_buffer_has_srgb || draw_buffers_has_srgb) &&
       ((mask & GL_COLOR_BUFFER_BIT) != 0);
   bool encode_srgb_only =
       (draw_buffers_has_srgb && !read_buffer_has_srgb) &&
       ((mask & GL_COLOR_BUFFER_BIT) != 0);
   if (!enable_srgb ||
       read_buffer_samples > 0 ||
       !feature_info_->feature_flags().desktop_srgb_support ||
       gl_version_info().IsAtLeastGL(4, 4) ||
@@ skipping 10516 matching lines @@
 }
 
 // Include the auto-generated part of this file. We split this because it means
 // we can easily edit the non-auto generated parts right here in this file
 // instead of having to edit some template or the code generator.
 #include "base/macros.h"
 #include "gpu/command_buffer/service/gles2_cmd_decoder_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu