[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;
     }
   }
 
+  gfx::Size read_size = GetBoundReadFramebufferSize();
+  gfx::Rect rect(0, 0, read_size.width(), read_size.height());
+  GLuint src_x = srcX1 > srcX0 ? srcX0 : srcX1;

[Zhenyao Mo, 2016/10/13 00:35:56]

From your code, it seems you include the lower bound, but exclude the upper
bound? and lower bound is defined as the smaller of the two values, not the
first value.  Is there any spec text to support such assumption?

[yunchao, 2016/10/13 01:50:47]

(Just a quick reply about this, I think the spec is not clear about this too). 
That's true, I use the smaller value as the lower bound. There are two reasons:
1) according to GLES3 man page
(https://www.khronos.org/opengles/sdk/docs/man3/html/glBlitFramebuffer.xhtml):
The lower bounds of the rectangle are inclusive, while the upper bounds are
exclusive. I think "the lower bounds of the rectangle" should be the small
value. 2) In many conformance tests, if both src region and dst region are
reversed, we think it equals to no reversion at all. If the lower left is
defined by (X0, Y0), this assumption is not correct. For example, considering
that blitting from (0, 0, 2, 2) to (0, 0, 2, 2) vs blitting from(2, 2, 0, 0) to
(2, 2, 0, 0), Pixel (0, 0) would be inclusive for the first case, but exclusive
for the second case. But (2, 2) would be exclusive for the first case, and it is
inclusive for the second case. 
WDYT? 

[Zhenyao Mo, 2016/10/13 22:51:23]

Yes, your interpretation sounds reasonable. The spec says pixels are half-pixel
coordinates, and only pixels whose centers are within the integer bounds are
written, so no matter which bound is specified first, it's always the the
smaller bound pixel included, and larger bound pixel excluded.

+  GLuint src_y = srcY1 > srcY0 ? srcY0 : srcY1;
+  GLuint src_width = srcX1 > srcX0 ? srcX1 - srcX0 : srcX0 - srcX1;

[Zhenyao Mo, 2016/10/13 00:35:56]

These math needs to be safe (using base::CheckedNumeric)

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

Done.

+  GLuint src_height = srcY1 > srcY0 ? srcY1 - srcY0 : srcY0 - srcY1;
+  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));
+    GLuint src_real_width = rect.width();
+    GLuint src_real_height = rect.height();
+    GLuint xoffset = rect.x() - src_x;

[Zhenyao Mo, 2016/10/13 00:35:56]

This could also cause overflow since src_x can be anything

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

Done.

+    GLuint yoffset = 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 = src_x + src_width - rect.x() - rect.width();
+    }
+    if (((srcY1 > srcY0) && (dstY1 < dstY0)) ||
+        ((srcY1 < srcY0) && (dstY1 > dstY0))) {
+      yoffset = src_y + src_height - rect.y() - rect.height();
+    }
+
+    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 = (filter == GL_LINEAR) ?

[yunchao, 2016/10/12 15:26:14]

Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 for
pixels lying on the border. 

[Zhenyao Mo, 2016/10/13 00:35:56]

I think no matter what the filter is, we should only sample from the pixels
within the effective src bound, and we should only write to the pixels that are
fully within the float dst bound.

[yunchao, 2016/10/13 01:50:47]

According to the GLES3 spec, for NEAREST filter, "Only pixels whose centers lie
within the destination rectangle are written by BlitFramebuffer". See page 197
at
https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.4.pdf#nameddest=s....
So I think std::round for LINEAR filter is correct. Do you mean the LINEAR
filter should follow this criteria too? 

[Corentin Wallez, 2016/10/13 14:35:53]

I understand the spec differently, see my comment on the Github PR. I believe
the "sample point in src rect" interpretation invalidate most of this CL.

[Zhenyao Mo, 2016/10/13 22:51:23]

cwallez: I fail to understand you here. I think what we are unclear is whether
to include or exclude the pixels on the bounds, but in general this CL is valid.
Why do you say most of this CL is invalid? Please elaborate.

[Zhenyao Mo, 2016/10/13 22:51:23]

yunchao: I think the spec you quoted is for both LINEAR and NEAREST. The
sampling outside the bounds only applies for src bounds, it has nothing to do
with deciding dst bounds. We should use the valid src bound to determine the
valid dst bound through simple scaling. Now any pixels whose centers are within
the valid dst bound should be written, regardless of what sampling filter is.

[Zhenyao Mo, 2016/10/13 23:16:15]

Thinking more about this: for the scaled valid low bound, if it's 1.1, it should
be 1 because we want to include pixel 1 whose center is 1.5 but exclude pixel 0
whose center is 0.5; if it's 1.6, it should be 2 beause we want to exclude pixel
1 whose center is 1.5 but include pixel 2 whose center is 2.5.  What's tricky is
if it's exactly 1.5, then pixel 1's center is 1.5, it can be included or
excluded. I suggest simply using round() to include it.

For the scaled valid high bound, if it's 10.1, it should be 10 because pixel 9's
center is 9.5 (included), pixel 10's center is 10.5 (excluded); if it's 10.6, it
should 11 because pixel 10's center is 10.5 (included) and pixel 11's center is
11.5 (excluded).  Again, if it's 10.5, it can either include or exclude pixel
10, whose center is exactly 10.5.  I suggest simply using round() to include it.

So per above analysis, you should always use round regardless of the filter or
lo/hi bounds.

[yunchao, 2016/10/14 02:52:50]

I agree with you, Zhenyao. I am just waiting for the final decision at this pr:
https://github.com/KhronosGroup/WebGL/pull/2098. Then I can update this patch
accordingly. 

In fact, my suggestion at that pr is exactly the same with you. We only blit the
pixels whose center lie in the effective bounds, no matter LINEAR/NEAREST
filter.  And I have updated my patch at local already: use std::round only in
the code snippet (from line 8032 to line 8037). 

[yunchao, 2016/10/14 02:52:50]

Yes, the most part of this patch should be OK, I have listed the unclear part in
the pr: https://github.com/KhronosGroup/WebGL/pull/2098. 

Corentin, I highly suggest you run the code. You can run the corresponding
conformance test w/ and w/o this patch (for example default_framebuffer_*.html)
to compare the rendering result. The patch works.

You can also run that pr I submitted on Linux/Windows machine. 

Moreover, you can look into the software rendering for blitFramebuffer by CPU in
C++ deqp (ReferenceContext::blitFramebuffer in sglrReferenceContext.cpp under
deqp/framework/opengl/simplereference/). 

[yunchao, 2016/10/14 02:52:50]

That's true. 

+        std::floor(dst_x + dst_mapping_xoffset) :
+        std::round(dst_x + dst_mapping_xoffset);
+    GLuint dst_mapping_y0 = (filter == GL_LINEAR) ?
+        std::floor(dst_y + dst_mapping_yoffset) :
+        std::round(dst_y + dst_mapping_yoffset);
+
+    GLuint dst_mapping_x1 = (filter == GL_LINEAR) ?
+        std::ceil(dst_x + dst_mapping_xoffset + dst_mapping_width) :
+        std::round(dst_x + dst_mapping_xoffset + dst_mapping_width);
+    GLuint dst_mapping_y1 = (filter == GL_LINEAR) ?
+        std::ceil(dst_y + dst_mapping_yoffset + dst_mapping_height) :
+        std::round(dst_y + dst_mapping_yoffset + dst_mapping_height);
+
+    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 10541 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