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

Issue 628866 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 429053



Sign in to add a comment

Restrict PACK_/UNPACK_ params to sane subrects

Project Member Reported by zmo@chromium.org, Jul 16 2016

Issue description

Copy from public_webgl mailing list, proposed by Jeff Gilbert from Mozzila

"""
Specifically, the following would generate INVALID_OPERATION:
* SKIP_PIXELS + width > ROW_LENGTH
* SKIP_ROWS + height > IMAGE_HEIGHT

This will:
* Make things cleaner for both implementers and devs
* Remove a source of implementation behavior differences
* Prevent a source of bugs for developers (setting SKIP_PIXELS without
ROW_LENGTH)

If we find a compelling use-case for relaxing this, no content
breakage will occur if we decide to loosen these restrictions later.
"""

"""
Unfettered behavior here, however, not make sense in the context of
the rest of the API.

There are two relevant cases:
* Overlapping rows
* Non-trivial subrects

Overlapping rows:
width>ROW_LENGTH
(The same applies to height>IMAGE_HEIGHT)
Overlapping rows are not tightly defined. As such, well-behaved apps
cannot expect the results of an overlapping row read to be consistent
across implementations. Since it's trivial to change the params into a
non-overlapped row configuration, with no loss of reliable utility, I
propose we restrict this behavior.

Nontrivial subrects:
SKIP_PIXELS+width>(ROW_LENGTH ? ROW_LENGTH : width)
(The same applies for SKIP_ROWS/IMAGE_HEIGHT)
If the goal is simply to offset into the buffer, there are other
entrypoints/API aspects which allow this.
However, there are weird results from using something like
SKIP_PIXEL=10000: Alignment (for the purposes of row striding) is
still based on effective row length alone. (effective row length =
ROW_LENGTH ? ROW_LENGTH : width) Thus `baseOffset + skipBytes +
N*rowStride` will not necessarily be aligned to `baseOffset +
M*ALIGNMENT`, subverting the ALIGNMENT param. (and generally being
suprising!)
Roughly, if `SKIP_PIXELS > ROW_LENGTH-width`, you can just offset the
base pointer via another part of the API, then make a call with
`SKIP_PIXELS <= ROW_LENGTH-width`.
Since there are other ways to handle arbitrary byte skipping, and
eliminating nontrivial subrects is an easy way to ensure expected
behavior, I propose we restrict this behavior.
"""

We need to update the spec, modify existing tests, and implement this behavior.
 

Comment 1 by zmo@chromium.org, Jul 18 2016

Pull request to change the spec and adjust the tests are here:
https://github.com/KhronosGroup/WebGL/pull/1903
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 19 2016

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

commit d0d61973aa559edd9c82afa109f032a243e45f88
Author: zmo <zmo@chromium.org>
Date: Tue Jul 19 04:36:45 2016

Implement WebGL2 PixelStorei params constraints.

BUG= 628866 
TEST=webgl2_conformance
R=piman@chromium.org
NOTRY=true
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

Review-Url: https://codereview.chromium.org/2154153002
Cr-Commit-Position: refs/heads/master@{#406214}

[modify] https://crrev.com/d0d61973aa559edd9c82afa109f032a243e45f88/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py
[modify] https://crrev.com/d0d61973aa559edd9c82afa109f032a243e45f88/gpu/command_buffer/client/gles2_implementation.cc
[modify] https://crrev.com/d0d61973aa559edd9c82afa109f032a243e45f88/gpu/command_buffer/client/gles2_implementation_unittest.cc

Comment 3 by zmo@chromium.org, Aug 4 2016

Owner: zmo@chromium.org
Status: Fixed (was: Available)
Components: -Internals>GPU>WebGL Blink>WebGL

Sign in to add a comment