Reland of Untouch out of bound pixels for CopyTexSubImage2D

Untouch out of bound pixels for CopyTexSubImage2D

CopyTexSubImage2D is defined not to touch the corresponding destination
range for any pixel outside the bound framebuffer. This CL removes code
which wrongly clears subregion of destination texture.

BUG=577357
Committed: https://crrev.com/98bcf19d91d046e9e04d820d9a8479e07b27834a
Cr-Commit-Position: refs/heads/master@{#370332}

[qiankun, 2016-01-14 15:05:29]

qiankun.miao@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org

[qiankun, 2016-01-14 15:05:30]

This CL fixes conformance/textures/misc/copy-tex-image-and-sub-image-2d.html.

[Zhenyao Mo, 2016-01-14 18:13:37]

https://codereview.chromium.org/1586053002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1586053002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11418: }
Look at the code above SetLevelClearedRect(), since now we leave out-of-bounds
pixels untouched, then we need to take that into consideration for the cleared
rect. Otherwise we could end up leaking vram.

[qiankun, 2016-01-19 16:07:32]

https://codereview.chromium.org/1586053002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1586053002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11418: }
On 2016/01/14 18:13:37, Zhenyao Mo wrote:
> Look at the code above SetLevelClearedRect(), since now we leave out-of-bounds
> pixels untouched, then we need to take that into consideration for the cleared
> rect. Otherwise we could end up leaking vram.

Thanks for pointing this. I updated the CL. PTAL.

[Zhenyao Mo, 2016-01-19 20:50:14]

The code looks good to me, but could you not modify the function args (xoffset,
yoffset, width, height) and use (destX, destY, copyWidth, copyHeight) instead? 
Modifying the original args semantic might cause misunderstanding and lead to
bugs in the future.

[qiankun, 2016-01-20 00:13:03]

On 2016/01/19 20:50:14, Zhenyao Mo wrote:
> The code looks good to me, but could you not modify the function args
(xoffset,
> yoffset, width, height) and use (destX, destY, copyWidth, copyHeight) instead?

> Modifying the original args semantic might cause misunderstanding and lead to
> bugs in the future.

Done.

[Zhenyao Mo, 2016-01-20 00:26:49]

https://codereview.chromium.org/1586053002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1586053002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11396: xoffset += (copyX - x);
What I mean is, do not modify xoffset, yoffset, width, and height here.

Declare destX, destY here, and in the CombineAdjacentRects(), use destX, destY,
copyWidth, copyHeight instead of xoffset, yoffset, width, height.

[qiankun, 2016-01-20 01:02:50]

https://codereview.chromium.org/1586053002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1586053002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11396: xoffset += (copyX - x);
On 2016/01/20 00:26:49, Zhenyao Mo wrote:
> What I mean is, do not modify xoffset, yoffset, width, and height here.
> 
> Declare destX, destY here, and in the CombineAdjacentRects(), use destX,
destY,
> copyWidth, copyHeight instead of xoffset, yoffset, width, height.

Done.

[Zhenyao Mo, 2016-01-20 01:05:00]

lgtm

Thanks for fixing this.

Is there a conformance test we can enable? If yes, please do so.

[qiankun, 2016-01-20 01:37:24]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-20 01:38:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586053002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586053002/60001

[qiankun, 2016-01-20 01:38:19]

On 2016/01/20 01:05:00, Zhenyao Mo wrote:
> lgtm
> 
> Thanks for fixing this.
> 
> Is there a conformance test we can enable? If yes, please do so.

Thanks for review. 
It fixes conformance/textures/misc/copy-tex-image-and-sub-image-2d.html.

[commit-bot: I haz the power, 2016-01-20 05:06:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 05:06:03]

Dry run: This issue passed the CQ dry run.

[qiankun, 2016-01-20 06:22:29]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2016-01-20 06:22:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586053002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586053002/60001

[commit-bot: I haz the power, 2016-01-20 06:27:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-20 06:28:47]

Description was changed from

==========
Untouch out of bound pixels for CopyTexSubImage2D

CopyTexSubImage2D is defined not to touch the corresponding destination
range for any pixel outside the bound framebuffer. This CL removes code
which wrongly clears subregion of destination texture.

BUG=577357
==========

to

==========
Untouch out of bound pixels for CopyTexSubImage2D

CopyTexSubImage2D is defined not to touch the corresponding destination
range for any pixel outside the bound framebuffer. This CL removes code
which wrongly clears subregion of destination texture.

BUG=577357

Committed: https://crrev.com/98bcf19d91d046e9e04d820d9a8479e07b27834a
Cr-Commit-Position: refs/heads/master@{#370332}
==========

[commit-bot: I haz the power, 2016-01-20 06:28:48]

Patchset 4 (id:??) landed as
https://crrev.com/98bcf19d91d046e9e04d820d9a8479e07b27834a
Cr-Commit-Position: refs/heads/master@{#370332}

[Justin Novosad, 2016-01-20 15:35:38]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1612473002/ by junov@chromium.org.

The reason for reverting is: This change breaks blink layout test
fast/canvas/webgl/uninitialized-test.html
Example of a failed run:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/5....

[qiankun, 2016-01-20 17:21:59]

On 2016/01/20 15:35:38, Justin Novosad wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/1612473002/ by mailto:junov@chromium.org.
> 
> The reason for reverting is: This change breaks blink layout test
> fast/canvas/webgl/uninitialized-test.html
> Example of a failed run:
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/5....

Thanks Justin.
I updated the layout test. PTAL.

[qiankun, 2016-01-20 17:23:42]

Description was changed from

==========
Untouch out of bound pixels for CopyTexSubImage2D

CopyTexSubImage2D is defined not to touch the corresponding destination
range for any pixel outside the bound framebuffer. This CL removes code
which wrongly clears subregion of destination texture.

BUG=577357

Committed: https://crrev.com/98bcf19d91d046e9e04d820d9a8479e07b27834a
Cr-Commit-Position: refs/heads/master@{#370332}
==========

to

==========
Untouch out of bound pixels for CopyTexSubImage2D

CopyTexSubImage2D is defined not to touch the corresponding destination
range for any pixel outside the bound framebuffer. This CL removes code
which wrongly clears subregion of destination texture.

BUG=577357

Committed: https://crrev.com/98bcf19d91d046e9e04d820d9a8479e07b27834a
Cr-Commit-Position: refs/heads/master@{#370332}
==========

[Zhenyao Mo, 2016-01-20 18:46:48]

On 2016/01/20 17:21:59, qiankun wrote:
> On 2016/01/20 15:35:38, Justin Novosad wrote:
> > A revert of this CL (patchset #4 id:60001) has been created in
> > https://codereview.chromium.org/1612473002/ by mailto:junov@chromium.org.
> > 
> > The reason for reverting is: This change breaks blink layout test
> > fast/canvas/webgl/uninitialized-test.html
> > Example of a failed run:
> >
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/5....
> 
> Thanks Justin.
> I updated the layout test. PTAL.

Qiankun: no need to further maintain the layout tests since they are just
duplicates of conformance tests.

I uploaded https://codereview.chromium.org/1601093008/ to remove them.  Once
that's landed and reviewed, we can reland your CL.