New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 859400 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Mac
Pri: 2
Type: Bug

Blocked on:
issue skia:2122

Blocking:
issue 662644


Participants' hotlists:
webgl-conformance-all


Sign in to add a comment

texSubImage3D generates invalid error

Project Member Reported by gman@chromium.org, Jul 2

Issue description

Chrome Version: 69.0.3478.0 (Official Build) canary (64-bit)
OS: Any? (Mac/Win)

What steps will reproduce the problem?
(1) http://jsfiddle.net/greggman/vwkhps65/

What is the expected result?

No error

What happens instead?

Gets an error


Works in Firefox



 
Cc: kainino@chromium.org jdarpinian@chromium.org
Labels: -Pri-3 OS-Mac OS-Windows Pri-2
Owner: zmo@chromium.org
Status: Assigned (was: Available)
I'll take a look at this bug.
Status: Started (was: Assigned)
Here is where this error is generated:

WebGL2RenderingContextBase::texSubImage3D ->
WebGLRenderingContextBase::TexImageHelperHTMLCanvasElement ->
WebGLRenderingContextBase::ValidateTexFunc

where we pass in canvas's width and height instead of the texSubImage3D's input param width/height.

So here canvas's height is 16512, whereas the max_texture_size is 16384.

I still need to understand why we use canvas's width/height for validation though.
Ken, I think in WebGLRenderingContextBase::TexImageHelperHTMLCanvasElement() where it calls ValidateTexFunc(), where we pass in canvas->width() and canvas->height(), we should pass in width and height instead.

However, I may misunderstand something here because using canvas's width/height seems intentional. Please take a look and advise before I change the code.
Yes, that code looks wrong. When adding support for WebGL 2.0, including uploads of sub-rectangles of DOM elements, and uploads of DOM elements to 3D textures and 2D texture arrays, the code became messy and this was missed.

It looks like there are more places where this validation is wrong. Looking at callers of ValidateTexFunc, it may also be wrong for ImageData, HTMLImageElement and HTMLVideoElement.

The difficulty is that the source sub-rectangle isn't always known when ValidateTexFunc is called. A "sentinel" empty rectangle is passed in in some cases. For example, for HTMLImageElement, image decoding (image->CachedImage()->GetImage()) has to be done in order to figure out the image's real width and height, so the callers of TexImageHelperHTMLImageElement in WebGLRenderingContextBase pass in an empty sub-rectangle so it's computed later. For the WebGL 2.0 entry points, the user's specified width and height are always used.

In short, it might be difficult to fix this bug for ImageData and HTMLImageElement, but the fix for 2D canvas looks good and correct.

Studied a bit of Mozilla's validation for uploading DOM elements to textures. It's structured interestingly, where more of the validation code is shared.

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/TexUnpackBlob.cpp?q=%2Bfunction%3A%22mozilla%3A%3Awebgl%3A%3ATexUnpackBlob%3A%3ATexUnpackBlob%28const+mozilla%3A%3AWebGLContext+%2A%2C+TexImageTarget%2C+uint32_t%2C+uint32_t%2C+uint32_t%2C+uint32_t%2C+gfxAlphaType%29%22&redirect_type=single#273

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/TexUnpackBlob.cpp?q=%2Bfunction%3A%22%28static+in+dom%2Fcanvas%2FTexUnpackBlob.cpp%29%3A%3Amozilla%3A%3Awebgl%3A%3AValidateUnpackPixels%28mozilla%3A%3AWebGLContext+%2A%2C+const+char+%2A%2C+uint32_t%2C+uint32_t%2C+webgl%3A%3ATexUnpackBlob+%2A%29%22&redirect_type=single#182

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3db41c3913fd21440c3f621722f353b2910b3257

commit 3db41c3913fd21440c3f621722f353b2910b3257
Author: Zhenyao Mo <zmo@chromium.org>
Date: Fri Jul 06 21:14:19 2018

Use the input width/height instead of canvas's width/height for tex func validation.

BUG= 859400 
TEST=webgl_conformance
R=kainino@chromium.org

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I5866530c90b5064177f5890c88eece2fb62f770e
Reviewed-on: https://chromium-review.googlesource.com/1125307
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Zhenyao Mo <zmo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573072}
[modify] https://crrev.com/3db41c3913fd21440c3f621722f353b2910b3257/third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc

Labels: TE-Verified-69.0.3486.0 TE-Verified-M69
Able to reproduce the issue on Mac 10.13.3 using chrome reported version #69.0.3478.0

Verified the fix on Mac 10.13.3 and win-10 using latest chrome version #69.0.3486.0 as per the comment #0.Attaching screen shot for reference.
Did not observe any error.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
Screen Shot 2018-07-09 at 17.13.37.png
628 KB View Download
BTW, here's the failure log from webgl2_conformance_gl_passthrough_tests on NVIDIA GPU on Windows:
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/win_angle_rel_ng/2139
https://chromium-swarm.appspot.com/task?id=3ed24e80e8183c10&refresh=10&show_raw=1

[41/136] gpu_tests.webgl_conformance_integration_test.WebGLConformanceIntegrationTest.WebglConformance_conformance2_textures_misc_tex_subimage3d_canvas_bug failed unexpectedly 1.2330s:
  
  Traceback (most recent call last):
    _RunGpuTest at content\test\gpu\gpu_tests\gpu_integration_test.py:132
      self.RunActualGpuTest(url, *args)
    RunActualGpuTest at content\test\gpu\gpu_tests\webgl_conformance_integration_test.py:185
      getattr(self, test_name)(test_path, *args[1:])
    _RunConformanceTest at content\test\gpu\gpu_tests\webgl_conformance_integration_test.py:199
      self._CheckTestCompletion()
    _CheckTestCompletion at content\test\gpu\gpu_tests\webgl_conformance_integration_test.py:195
      self.fail(self._WebGLTestMessages(self.tab))
    fail at .swarming_module\bin\Lib\unittest\case.py:410
      raise self.failureException(msg)
  AssertionError: getError expected: NO_ERROR. Was INVALID_VALUE : Should be no errors from TexSubImage3D.
  FAIL getError expected: NO_ERROR. Was INVALID_VALUE : Should be no errors from TexSubImage3D.

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f64860d89fdb6d3397286e722b42f18c7df7db7

commit 6f64860d89fdb6d3397286e722b42f18c7df7db7
Author: Kenneth Russell <kbr@chromium.org>
Date: Sat Jul 21 02:54:54 2018

Roll WebGL a5c263c..21dbf06

https://chromium.googlesource.com/external/khronosgroup/webgl.git/+log/a5c263c..21dbf06

Bug:  830901 , 851870, 851873,  859400 ,  866089 
Tbr: zmo@chromium.org
Tbr: kainino@chromium.org
Cq-Include-Trybots: luci.chromium.try:win_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_angle_rel_ng;luci.chromium.try:win_angle_rel_ng
Change-Id: Icef8ddc970ccb6653e95920c28b4d003d1b27100
Reviewed-on: https://chromium-review.googlesource.com/1144306
Commit-Queue: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577063}
[modify] https://crrev.com/6f64860d89fdb6d3397286e722b42f18c7df7db7/DEPS
[modify] https://crrev.com/6f64860d89fdb6d3397286e722b42f18c7df7db7/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py
[modify] https://crrev.com/6f64860d89fdb6d3397286e722b42f18c7df7db7/content/test/gpu/gpu_tests/webgl_conformance_revision.txt

Blocking: 662644
Labels: webgl-conformance
Owner: jdarpinian@chromium.org
The test assumes we can can create a 2D canvas larger than MAX_TEXTURE_SIZE, but we limit 2D canvas size to 32768, so on platforms where MAX_TEXTURE_SIZE is >= 32768 this test fails. I'm not sure what to do about it yet.
Blockedon: skia:2122
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5fd81121bab7d735ed01a8a112df253807c2e77c

commit 5fd81121bab7d735ed01a8a112df253807c2e77c
Author: James Darpinian <jdarpinian@chromium.org>
Date: Mon Dec 03 20:37:16 2018

Increase maximum width/height of canvas.

Fixes WebGL conformance test:
conformance2/textures/misc/tex-subimage3d-canvas-bug.html

The test expects to be able to create a canvas larger than 32767 in
height. Skia seems to support this, and there have been user requests
for it.

Bug:  859400 ,  339725 , skia:2122
Change-Id: Ibba89e98178bd73817c7b4cbdf569e89ee5764b1
Reviewed-on: https://chromium-review.googlesource.com/c/1356555
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: James Darpinian <jdarpinian@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613225}
[modify] https://crrev.com/5fd81121bab7d735ed01a8a112df253807c2e77c/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py
[modify] https://crrev.com/5fd81121bab7d735ed01a8a112df253807c2e77c/third_party/blink/renderer/platform/graphics/skia/skia_utils.h
[modify] https://crrev.com/5fd81121bab7d735ed01a8a112df253807c2e77c/third_party/blink/web_tests/fast/canvas/canvas-large-pattern.html

Status: Fixed (was: Started)
Cc: fs...@chromium.org phanindra.mandapaka@chromium.org manoranj...@chromium.org abdulsyed@chromium.org
 Issue 861784  has been merged into this issue.

Sign in to add a comment