Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2409523002:
[Command buffer] Fix the bug when blitting pixels outside read framebuffer (Closed)
Description[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/b6c1222980a7ed3761601d4a5455fa366eb134f6
Cr-Commit-Position: refs/heads/master@{#426155}
Patch Set 1 #Patch Set 2 : update webgl2 expectation #Patch Set 3 : Fix bots failure #Patch Set 4 : Fix the error when coordinates are reversed #Patch Set 5 : code rebase #Patch Set 6 : code rebase again #Patch Set 7 : Code refactoring #
Total comments: 18
Patch Set 8 : make the fix under workarounds #Patch Set 9 : Addressed feedbacks from Zhenyao and Corentin #Patch Set 10 : code rebase #
Total comments: 14
Patch Set 11 : Addressed feedbacks from Corentin and Zhenyao #
Total comments: 6
Patch Set 12 : code rebase, and addressed one more feedback from Corentin #Patch Set 13 : Addressed zmo@'s feedback #Patch Set 14 : code rebase again #
Total comments: 2
Patch Set 15 : Addressed Corentin's feedback #
Messages
Total messages: 99 (72 generated)
Description was changed from ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer BUG=644740 ========== to ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer BUG=644740 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
Description was changed from ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer BUG=644740 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer BUG=644740 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
yunchao.he@intel.com changed reviewers: + kbr@chromium.org, piman@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
Handle the situation when blitting out-of-bounds. PTAL. Thanks a lot. Maybe the code should be wrapped into a workaround... https://codereview.chromium.org/2409523002/diff/120001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2409523002/diff/120001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:198: ['mac', 'nvidia'], bug=483282) Don't know why only default_framebuffer_04 on Mac Nvidia still fail(bots told me this failure). But all other tests against blitting out-of-bounds can pass on Mac Intel/NVidia/AMD and on Linux Intel/AMD. And I can not reproduce this issue because I have no Mac Nvidia device at my side. Maybe we can file a bug specific for Mac Nvidia for this only failure.
One more comment for the pixels lying on the border. https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 for pixels lying on the border.
https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7995: GLuint src_x = srcX1 > srcX0 ? srcX0 : srcX1; 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? https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7997: GLuint src_width = srcX1 > srcX0 ? srcX1 - srcX0 : srcX0 - srcX1; These math needs to be safe (using base::CheckedNumeric) https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8006: GLuint xoffset = rect.x() - src_x; This could also cause overflow since src_x can be anything https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/12 15:26:14, yunchao wrote: > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 for > pixels lying on the border. 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.
Thanks for your review, Zhenyao. Some feedbacks from me about the unclear part of this issue are inline. Could you take a look? https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7995: GLuint src_x = srcX1 > srcX0 ? srcX0 : srcX1; On 2016/10/13 00:35:56, Zhenyao Mo wrote: > 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? (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? https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/13 00:35:56, Zhenyao Mo wrote: > On 2016/10/12 15:26:14, yunchao wrote: > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 for > > pixels lying on the border. > > 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. 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?
On 2016/10/13 01:50:47, yunchao wrote: > Thanks for your review, Zhenyao. Some feedbacks from me about the unclear part > of this issue are inline. Could you take a look? > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:7995: GLuint src_x = srcX1 > > srcX0 ? srcX0 : srcX1; > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > 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? > > (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? > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = > (filter == GL_LINEAR) ? > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > On 2016/10/12 15:26:14, yunchao wrote: > > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 > for > > > pixels lying on the border. > > > > 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. > > 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 Sorry, correct a typo, there should be NEAREST > filter is correct. Do you mean the LINEAR > filter should follow this criteria too? Correct a typo in the comment, sorry...
On 2016/10/13 at 02:00:40, yunchao.he wrote: > On 2016/10/13 01:50:47, yunchao wrote: > > Thanks for your review, Zhenyao. Some feedbacks from me about the unclear part > > of this issue are inline. Could you take a look? > > > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:7995: GLuint src_x = srcX1 > > > srcX0 ? srcX0 : srcX1; > > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > > 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? > > > > (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? > > > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = > > (filter == GL_LINEAR) ? > > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > > On 2016/10/12 15:26:14, yunchao wrote: > > > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 > > for > > > > pixels lying on the border. > > > > > > 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. > > > > 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 > Sorry, correct a typo, there should be NEAREST > > > filter is correct. Do you mean the LINEAR > > filter should follow this criteria too? > > Correct a typo in the comment, sorry... Not lgtm, we should resolve the spec interpretation before moving on.
cwallez@chromium.org changed reviewers: + cwallez@chromium.org
https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/13 at 01:50:47, yunchao wrote: > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > On 2016/10/12 15:26:14, yunchao wrote: > > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 for > > > pixels lying on the border. > > > > 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. > > 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? 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.
https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7995: GLuint src_x = srcX1 > srcX0 ? srcX0 : srcX1; 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. https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/13 01:50:47, yunchao wrote: > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > On 2016/10/12 15:26:14, yunchao wrote: > > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 > for > > > pixels lying on the border. > > > > 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. > > 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? 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. https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/13 14:35:53, Corentin Wallez wrote: > On 2016/10/13 at 01:50:47, yunchao wrote: > > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > > On 2016/10/12 15:26:14, yunchao wrote: > > > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 > for > > > > pixels lying on the border. > > > > > > 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. > > > > 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? > > 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. 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.
On 2016/10/13 at 22:51:23, zmo wrote: > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:7995: GLuint src_x = srcX1 > srcX0 ? srcX0 : srcX1; > 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. > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? > On 2016/10/13 01:50:47, yunchao wrote: > > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > > On 2016/10/12 15:26:14, yunchao wrote: > > > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 > > for > > > > pixels lying on the border. > > > > > > 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. > > > > 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? > > 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. > > https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? > On 2016/10/13 14:35:53, Corentin Wallez wrote: > > On 2016/10/13 at 01:50:47, yunchao wrote: > > > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > > > On 2016/10/12 15:26:14, yunchao wrote: > > > > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 > > for > > > > > pixels lying on the border. > > > > > > > > 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. > > > > > > 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? > > > > 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. > > 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. I meant to say that the CL should be updated to reflect the spec clarification. I will re-review the rest of the CL tomorrow, apologies for the delay.
https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/13 22:51:23, Zhenyao Mo wrote: > On 2016/10/13 01:50:47, yunchao wrote: > > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > > On 2016/10/12 15:26:14, yunchao wrote: > > > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 > > for > > > > pixels lying on the border. > > > > > > 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. > > > > 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? > > 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. 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.
Zhenyao and Corentein, thanks for your feedbacks. Below are some comments inline. https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/13 00:35:56, Zhenyao Mo wrote: > On 2016/10/12 15:26:14, yunchao wrote: > > Please see the discuss at https://github.com/KhronosGroup/WebGL/pull/2070 for > > pixels lying on the border. > > 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. That's true. https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/13 23:16:15, Zhenyao Mo wrote: > On 2016/10/13 22:51:23, Zhenyao Mo wrote: > > On 2016/10/13 01:50:47, yunchao wrote: > > > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > > > On 2016/10/12 15:26:14, yunchao wrote: > > > > > Please see the discuss at > https://github.com/KhronosGroup/WebGL/pull/2070 > > > for > > > > > pixels lying on the border. > > > > > > > > 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. > > > > > > 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? > > > > 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. > > 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. 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). https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8032: GLuint dst_mapping_x0 = (filter == GL_LINEAR) ? On 2016/10/13 22:51:23, Zhenyao Mo wrote: > On 2016/10/13 14:35:53, Corentin Wallez wrote: > > On 2016/10/13 at 01:50:47, yunchao wrote: > > > On 2016/10/13 00:35:56, Zhenyao Mo wrote: > > > > On 2016/10/12 15:26:14, yunchao wrote: > > > > > Please see the discuss at > https://github.com/KhronosGroup/WebGL/pull/2070 > > for > > > > > pixels lying on the border. > > > > > > > > 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. > > > > > > 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? > > > > 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. > > 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. 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/).
Patchset #10 (id:180001) has been deleted
Addressed feedbacks from Zhenyao and Corentin. https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7997: GLuint src_width = srcX1 > srcX0 ? srcX1 - srcX0 : srcX0 - srcX1; On 2016/10/13 00:35:56, Zhenyao Mo wrote: > These math needs to be safe (using base::CheckedNumeric) Done. https://codereview.chromium.org/2409523002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8006: GLuint xoffset = rect.x() - src_x; On 2016/10/13 00:35:56, Zhenyao Mo wrote: > This could also cause overflow since src_x can be anything Done. https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8039: GLuint dst_mapping_x0 = Use std::round for both NEAREST and LINEAR filter.
Description was changed from ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer BUG=644740 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer. All related failures in deqp/ and conformance2/ can be fixed by this change. BUG=644740 TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html TEST=conformance2/rendering/blitframebuffer_filter_outofbounds.html, TEST=deqp/functional/gels3/framebufferblit/rect_02.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer. All related failures in deqp/ and conformance2/ can be fixed by this change. BUG=644740 TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html TEST=conformance2/rendering/blitframebuffer_filter_outofbounds.html, TEST=deqp/functional/gels3/framebufferblit/rect_02.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer. All related failures in deqp/ and conformance2/ can be fixed by this change. BUG=644740 TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html, TEST=conformance2/rendering/blitframebuffer_filter_outofbounds.html, TEST=deqp/functional/gels3/framebufferblit/rect_02.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer. All related failures in deqp/ and conformance2/ can be fixed by this change. BUG=644740 TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html, TEST=conformance2/rendering/blitframebuffer_filter_outofbounds.html, TEST=deqp/functional/gels3/framebufferblit/rect_02.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command buffer] Fix the bug when blitting pixels outside read framebuffer. All related failures in deqp/ and conformance2/ can be fixed by this change. BUG=644740 TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html, TEST=conformance2/rendering/blitframebuffer_filter_outofbounds.html, TEST=deqp/functional/gels3/framebufferblit/rect_02.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7995: gfx::Rect rect(0, 0, read_size.width(), read_size.height()); rect isn't a very descriptive name, use "source_bounds" or something? https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8002: GLuint src_width = src_width_temp.ValueOrDie(); 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. https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8008: rect.Intersect(gfx::Rect(src_x, src_y, src_width, src_height)); rect.Intersect(src_region) https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8023: GLuint xoffset = xoffset_temp.ValueOrDie(); Same as above for the CheckedNumerics
https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7999: srcX1 > srcX0 ? srcX1 - srcX0 : srcX0 - srcX1; 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) https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8002: GLuint src_width = src_width_temp.ValueOrDie(); 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.
https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8002: GLuint src_width = src_width_temp.ValueOrDie(); On 2016/10/14 17:54:15, Zhenyao Mo wrote: > 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. 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.
Patchset #11 (id:220001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for your review, Corentin and Zhenyao. I have updated the code according to your feedback. Please take a look. Thanks a lot! https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7995: gfx::Rect rect(0, 0, read_size.width(), read_size.height()); On 2016/10/14 14:42:42, Corentin Wallez wrote: > rect isn't a very descriptive name, use "source_bounds" or something? That's true. I have updated the name accordingly. https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7999: srcX1 > srcX0 ? srcX1 - srcX0 : srcX0 - srcX1; On 2016/10/14 17:54:15, Zhenyao Mo wrote: > 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) Yes, you are correct, Zhenyao. The overflow happens during "-" or "+", not "='. The other places do it in the same way as you suggested. https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8002: GLuint src_width = src_width_temp.ValueOrDie(); On 2016/10/14 14:42:42, Corentin Wallez wrote: > 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. Done. https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8002: GLuint src_width = src_width_temp.ValueOrDie(); On 2016/10/14 21:10:32, Zhenyao Mo wrote: > On 2016/10/14 17:54:15, Zhenyao Mo wrote: > > 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. > > 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. Done. https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8008: rect.Intersect(gfx::Rect(src_x, src_y, src_width, src_height)); On 2016/10/14 14:42:42, Corentin Wallez wrote: > rect.Intersect(src_region) Yeah... Will update the code here. https://codereview.chromium.org/2409523002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8023: GLuint xoffset = xoffset_temp.ValueOrDie(); On 2016/10/14 14:42:42, Corentin Wallez wrote: > Same as above for the CheckedNumerics Done https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8020: GLuint xoffset = src_bounds.x() - src_x; Thought about the calculation here. Now that the width/height of src region are valid. The computation here will not overflow. https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8026: xoffset = src_x + src_width - src_bounds.x() - src_bounds.width(); Not overflow for every step during "+" and "-", considering that the width/height of src region are valid.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry another tiny bug I didn't catch in previous review https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7997: GLuint src_y = srcY1 > srcY0 ? srcY0 : srcY1; These two needs to be GLint. https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8034: GLuint dst_y = dstY1 > dstY0 ? dstY0 : dstY1; Same here: these two needs to be GLint.
Thanks for your careful review, Zhenyao. Code has been updated accordingly. Please take another look. Thanks a lot! https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7997: GLuint src_y = srcY1 > srcY0 ? srcY0 : srcY1; On 2016/10/17 20:16:21, Zhenyao Mo wrote: > These two needs to be GLint. Done. https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8034: GLuint dst_y = dstY1 > dstY0 ? dstY0 : dstY1; On 2016/10/17 20:16:21, Zhenyao Mo wrote: > Same here: these two needs to be GLint. Done.
On 2016/10/17 23:51:29, yunchao wrote: > Thanks for your careful review, Zhenyao. Code has been updated accordingly. > Please take another look. Thanks a lot! > > https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:7997: GLuint src_y = srcY1 > > srcY0 ? srcY0 : srcY1; > On 2016/10/17 20:16:21, Zhenyao Mo wrote: > > These two needs to be GLint. > > Done. > > https://codereview.chromium.org/2409523002/diff/240001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8034: GLuint dst_y = dstY1 > > dstY0 ? dstY0 : dstY1; > On 2016/10/17 20:16:21, Zhenyao Mo wrote: > > Same here: these two needs to be GLint. > > Done. lgtm
Corentin, could you also take another look? Thanks a lot! @cwallez.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
On 2016/10/18 at 00:26:56, yunchao.he wrote: > Corentin, could you also take another look? Thanks a lot! @cwallez. LGTM one minor nit.
https://codereview.chromium.org/2409523002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2409523002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8009: } else { nit: no need for an else here. https://codereview.chromium.org/2409523002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:8046: } else { nit: ditto
Description was changed from
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
TEST=conformance2/rendering/blitframebuffer_filter_outofbounds.html,
TEST=deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
Description was changed from
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
Description was changed from
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #15 (id:320001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by yunchao.he@intel.com
On 2016/10/18 20:13:19, Corentin Wallez wrote: > https://codereview.chromium.org/2409523002/diff/300001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2409523002/diff/300001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8009: } else { > nit: no need for an else here. > > https://codereview.chromium.org/2409523002/diff/300001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:8046: } else { > nit: ditto Thanks for your review, Corentin. Merging the updated code now...
The CQ bit was checked by yunchao.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org, cwallez@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2409523002/#ps340001 (title: "Addressed Corentin's feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
Message was sent while issue was closed.
Committed patchset #15 (id:340001)
Message was sent while issue was closed.
Description was changed from
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
[Command buffer] Fix the bug when blitting pixels outside read framebuffer.
All related failures in deqp/ and conformance2/ can be fixed by this change.
BUG=644740
TEST=deqp/functional/gles3/framebufferblit/default_framebuffer_*.html,
conformance2/rendering/blitframebuffer_filter_outofbounds.html,
deqp/functional/gels3/framebufferblit/rect_02.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/b6c1222980a7ed3761601d4a5455fa366eb134f6
Cr-Commit-Position: refs/heads/master@{#426155}
==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/b6c1222980a7ed3761601d4a5455fa366eb134f6 Cr-Commit-Position: refs/heads/master@{#426155}
Message was sent while issue was closed.
Yang is OOO from Oct 21 to 30, please expect slow response. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
