Fix unpacking overlapping unpack buffer rows on NVIDIA GL

Fix unpacking overlapping unpack buffer rows on NVIDIA GL

When unpack parameters are set so that the rows being read overlap in
the unpack buffer stored in GPU memory, NVIDIA GL driver may not
upload the last pixels of the last one or more rows of a texture. The
driver may also crash when the amount of overlap is high.

This issue affects both TexImage* and TexSubImage* calls.

Work around the issue by uploading textures row by row when the rows
being read overlap in the unpack buffer. The workaround could possibly
be optimized by uploading several of the first rows with a single call
in some cases where the amount of overlap is low, but this is expected
to be a rarely used corner case, so the added complexity that the
optimization would create seems like a bad tradeoff.

The issue does not seem to be triggered when the layers (images) of a
3D texture overlap, as long as rows inside the images don't.

BUG=596774
TEST=WebGL 2 conformance tests
     conformance2/textures/misc/tex-unpack-params.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/0a55756ea0c674213d6d137d26f5998a89c300ee
Cr-Commit-Position: refs/heads/master@{#392581}

[oetuaho-nv, 2016-05-09 14:13:45]

Description was changed from

==========
Fix unpacking overlapping unpack buffer rows on NVIDIA GL

When unpack parameters are set so that the rows being read overlap in
the unpack buffer stored in GPU memory, NVIDIA GL driver may not
upload the last pixels of the last one or more rows of a texture. The
driver may also crash when the amount of overlap is high.

This issue affects both TexImage* and TexSubImage* calls.

Work around the issue by uploading textures row by row when the rows
being read overlap in the unpack buffer. The workaround could possibly
be optimized by uploading several of the first rows with a single call
in some cases where the amount of overlap is low, but this is expected
to be a rarely used corner case, so the added complexity that the
optimization would create seems like a bad tradeoff.

The issue does not seem to be triggered when the layers (images) of a
3D texture overlap, as long as rows inside the images don't.

BUG=596774
TEST=WebGL 2 conformance tests
     conformance2/textures/misc/tex-unpack-params.html
==========

to

==========
Fix unpacking overlapping unpack buffer rows on NVIDIA GL

When unpack parameters are set so that the rows being read overlap in
the unpack buffer stored in GPU memory, NVIDIA GL driver may not
upload the last pixels of the last one or more rows of a texture. The
driver may also crash when the amount of overlap is high.

This issue affects both TexImage* and TexSubImage* calls.

Work around the issue by uploading textures row by row when the rows
being read overlap in the unpack buffer. The workaround could possibly
be optimized by uploading several of the first rows with a single call
in some cases where the amount of overlap is low, but this is expected
to be a rarely used corner case, so the added complexity that the
optimization would create seems like a bad tradeoff.

The issue does not seem to be triggered when the layers (images) of a
3D texture overlap, as long as rows inside the images don't.

BUG=596774
TEST=WebGL 2 conformance tests
     conformance2/textures/misc/tex-unpack-params.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[oetuaho-nv, 2016-05-09 14:22:29]

oetuaho@nvidia.com changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[oetuaho-nv, 2016-05-09 14:22:30]

Can you take a quick look? This workaround should be pretty solid in terms of
functionality, I verified this by manually running relevant tests.

I couldn't run the WebGL 2 tests on the Linux GN build with the test runner
script, it seems to complain that Chromium build can't be found.

I tried:

content/test/gpu/run_gpu_test webgl_conformance --webgl-conformance-version=2.0

Is this correct?

Some comments inline as well.

https://codereview.chromium.org/1963683002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1963683002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/texture_manager.cc:2410: // filled with TexSubImage.
This part here could maybe be refactored into a common helper function.

https://codereview.chromium.org/1963683002/diff/1/gpu/config/gpu_driver_bug_l...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/1963683002/diff/1/gpu/config/gpu_driver_bug_l...
gpu/config/gpu_driver_bug_list_json.cc:2001: "gl_vendor": "NVIDIA.*",
Should this be sufficient to only trigger the workaround on GL driver? Or should
I add this workaround separately for Linux, Android, OSX?

[Ken Russell (switch to Gerrit), 2016-05-09 18:33:42]

On 2016/05/09 14:22:30, oetuaho-nv wrote:
> Can you take a quick look? This workaround should be pretty solid in terms of
> functionality, I verified this by manually running relevant tests.
> 
> I couldn't run the WebGL 2 tests on the Linux GN build with the test runner
> script, it seems to complain that Chromium build can't be found.
> 
> I tried:
> 
> content/test/gpu/run_gpu_test webgl_conformance
--webgl-conformance-version=2.0
> 
> Is this correct?

Into what directory are you generating your build? If the standard "out/Debug"
or "out/Release" then --browser=debug or --browser=release should work fine.
Also, I've been using "--webgl-conformance-version=2.0.0" but not sure whether
the other works.



> 
> Some comments inline as well.
> 
>
https://codereview.chromium.org/1963683002/diff/1/gpu/command_buffer/service/...
> File gpu/command_buffer/service/texture_manager.cc (right):
> 
>
https://codereview.chromium.org/1963683002/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/texture_manager.cc:2410: // filled with
TexSubImage.
> This part here could maybe be refactored into a common helper function.
> 
>
https://codereview.chromium.org/1963683002/diff/1/gpu/config/gpu_driver_bug_l...
> File gpu/config/gpu_driver_bug_list_json.cc (right):
> 
>
https://codereview.chromium.org/1963683002/diff/1/gpu/config/gpu_driver_bug_l...
> gpu/config/gpu_driver_bug_list_json.cc:2001: "gl_vendor": "NVIDIA.*",
> Should this be sufficient to only trigger the workaround on GL driver? Or
should
> I add this workaround separately for Linux, Android, OSX?

[Ken Russell (switch to Gerrit), 2016-05-09 18:34:09]

This lgtm overall but I am not an owner; Mo should review.

https://codereview.chromium.org/1963683002/diff/1/gpu/config/gpu_driver_bug_l...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/1963683002/diff/1/gpu/config/gpu_driver_bug_l...
gpu/config/gpu_driver_bug_list_json.cc:2001: "gl_vendor": "NVIDIA.*",
On 2016/05/09 14:22:30, oetuaho-nv wrote:
> Should this be sufficient to only trigger the workaround on GL driver? Or
should
> I add this workaround separately for Linux, Android, OSX?

This should be sufficient. If one or more of the platforms fixes the bug and we
decide to disable the workaround, we might need to add separate entries, but
let's do that later.

[Zhenyao Mo, 2016-05-09 19:57:17]

Thanks for working on this.

LGTM

Could you please also enable the conformance test together with this workaround?

[oetuaho-nv, 2016-05-10 08:30:21]

The CQ bit was checked by oetuaho@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-10 08:30:34]

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

[commit-bot: I haz the power, 2016-05-10 10:59:26]

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

[commit-bot: I haz the power, 2016-05-10 10:59:27]

Dry run: This issue passed the CQ dry run.

[oetuaho-nv, 2016-05-10 11:32:51]

Enabled the test on Linux, looks like on some other platforms it still needs to
be disabled. Committing.

[oetuaho-nv, 2016-05-10 11:32:57]

The CQ bit was checked by oetuaho@nvidia.com

[oetuaho-nv, 2016-05-10 11:32:58]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1963683002/#ps20001
(title: "Slight refactoring and change Linux expectations")

[commit-bot: I haz the power, 2016-05-10 11:33:12]

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

[commit-bot: I haz the power, 2016-05-10 11:36:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-10 11:38:17]

Description was changed from

==========
Fix unpacking overlapping unpack buffer rows on NVIDIA GL

When unpack parameters are set so that the rows being read overlap in
the unpack buffer stored in GPU memory, NVIDIA GL driver may not
upload the last pixels of the last one or more rows of a texture. The
driver may also crash when the amount of overlap is high.

This issue affects both TexImage* and TexSubImage* calls.

Work around the issue by uploading textures row by row when the rows
being read overlap in the unpack buffer. The workaround could possibly
be optimized by uploading several of the first rows with a single call
in some cases where the amount of overlap is low, but this is expected
to be a rarely used corner case, so the added complexity that the
optimization would create seems like a bad tradeoff.

The issue does not seem to be triggered when the layers (images) of a
3D texture overlap, as long as rows inside the images don't.

BUG=596774
TEST=WebGL 2 conformance tests
     conformance2/textures/misc/tex-unpack-params.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix unpacking overlapping unpack buffer rows on NVIDIA GL

When unpack parameters are set so that the rows being read overlap in
the unpack buffer stored in GPU memory, NVIDIA GL driver may not
upload the last pixels of the last one or more rows of a texture. The
driver may also crash when the amount of overlap is high.

This issue affects both TexImage* and TexSubImage* calls.

Work around the issue by uploading textures row by row when the rows
being read overlap in the unpack buffer. The workaround could possibly
be optimized by uploading several of the first rows with a single call
in some cases where the amount of overlap is low, but this is expected
to be a rarely used corner case, so the added complexity that the
optimization would create seems like a bad tradeoff.

The issue does not seem to be triggered when the layers (images) of a
3D texture overlap, as long as rows inside the images don't.

BUG=596774
TEST=WebGL 2 conformance tests
     conformance2/textures/misc/tex-unpack-params.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/0a55756ea0c674213d6d137d26f5998a89c300ee
Cr-Commit-Position: refs/heads/master@{#392581}
==========

[commit-bot: I haz the power, 2016-05-10 11:38:18]

Patchset 2 (id:??) landed as
https://crrev.com/0a55756ea0c674213d6d137d26f5998a89c300ee
Cr-Commit-Position: refs/heads/master@{#392581}