Implement helper functionalities for computing image size with ES3 settings.

Implement helper functionalities for computing image size with ES3 settings.

This will be used in ReadPixels, Tex{Sub}Image* in followup CLs.

BUG=429053, 563714
TEST=gpu_unittests
R=piman@chromium.org

Committed: https://crrev.com/e0b58d1f24bce6a6f19dcbe5e857e3731d0631ca
Cr-Commit-Position: refs/heads/master@{#364095}

[Zhenyao Mo, 2015-12-07 22:49:20]

piman: PTAL
yunchao: FYI (This will unblock https://codereview.chromium.org/1320093002/)

[Zhenyao Mo, 2015-12-07 22:49:25]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[Zhenyao Mo, 2015-12-07 22:51:53]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 22:51:54]

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

[Zhenyao Mo, 2015-12-07 22:53:39]

Description was changed from

==========
Implement helper functionalities for computing image size with ES3 settings.

This will be used in ReadPixels, Tex{Sub}Image* in followup CLs.

BUG=429053,563714
TEST=gpu_unittests
R=piman@chromium.org
==========

to

==========
Implement helper functionalities for computing image size with ES3 settings.

This will be used in ReadPixels, Tex{Sub}Image* in followup CLs.

BUG=429053,563714
TEST=gpu_unittests
R=piman@chromium.org
==========

[Zhenyao Mo, 2015-12-07 22:59:47]

Patchset #1 (id:1) has been deleted

[piman, 2015-12-08 00:06:11]

https://codereview.chromium.org/1508953002/diff/20001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils.cc (right):

https://codereview.chromium.org/1508953002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:574: int actual_width =
params.row_length > 0 ? params.row_length : width;
So, technically, the code (whether client-side, or GL, depending on the case)
will not access the trailing bytes. For example for width==height==4,
1-byte-per-pixel format, and a (un)pack_row_length of 8, the code would only
access 8 bytes on each of the first 3 row, and then 4 bytes in the last row.

So the rule is not height*pack_row_length, but (height-1)*pack_row_length +
width.
See also the code on l.551 and following above, that treats the last row
specially.

This is actually critical for the case where pack_row_length is less than the
width. In the case above, if pack_row_length is 2, then you still need to count
the last line fully - 3*2+4==10 instead of 4*2==8.

This is similar for actual_height vs (un)pack_image_height - the formula is
(depth-1)*pack_image_height +  (height-1)*pack_row_length + width.

The interesting bit is that then you don't care about whether the transfer is 2D
or 3D (so you wouldn't need ContextState::Dimension). If depth==1, then the
formula above doesn't depend on pack_image_height.

https://codereview.chromium.org/1508953002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:605: if (!SafeAddUint32(*size,
total_skip_size, size))
I think this is more confusing than useful. Only |size| bytes are used, it's
just that they're offset to |total_skip_size|.

In particular if we want to use this to size the shared memory, we shouldn't
count the skipped bytes (since they're skipped on the client side).

[commit-bot: I haz the power, 2015-12-08 00:52:29]

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

[commit-bot: I haz the power, 2015-12-08 00:52:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhenyao Mo, 2015-12-08 23:39:18]

https://codereview.chromium.org/1508953002/diff/20001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils.cc (right):

https://codereview.chromium.org/1508953002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:574: int actual_width =
params.row_length > 0 ? params.row_length : width;
On 2015/12/08 00:06:10, piman (slow to review) wrote:
> So, technically, the code (whether client-side, or GL, depending on the case)
> will not access the trailing bytes. For example for width==height==4,
> 1-byte-per-pixel format, and a (un)pack_row_length of 8, the code would only
> access 8 bytes on each of the first 3 row, and then 4 bytes in the last row.
> 
> So the rule is not height*pack_row_length, but (height-1)*pack_row_length +
> width.
> See also the code on l.551 and following above, that treats the last row
> specially.
> 
> This is actually critical for the case where pack_row_length is less than the
> width. In the case above, if pack_row_length is 2, then you still need to
count
> the last line fully - 3*2+4==10 instead of 4*2==8.
> 
> This is similar for actual_height vs (un)pack_image_height - the formula is
> (depth-1)*pack_image_height +  (height-1)*pack_row_length + width.
> 
> The interesting bit is that then you don't care about whether the transfer is
2D
> or 3D (so you wouldn't need ContextState::Dimension). If depth==1, then the
> formula above doesn't depend on pack_image_height.

Done.

However, I think we still need the ContextState::Dimension arg to avoid applying
skip_images in 2D.

https://codereview.chromium.org/1508953002/diff/20001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.cc:605: if (!SafeAddUint32(*size,
total_skip_size, size))
On 2015/12/08 00:06:10, piman (slow to review) wrote:
> I think this is more confusing than useful. Only |size| bytes are used, it's
> just that they're offset to |total_skip_size|.
> 
> In particular if we want to use this to size the shared memory, we shouldn't
> count the skipped bytes (since they're skipped on the client side).

OK, I won't write into size, but we still wanna check overflow for this add.

[Zhenyao Mo, 2015-12-08 23:40:38]

Patchset #2 (id:40001) has been deleted

[Zhenyao Mo, 2015-12-08 23:40:58]

Revised.  Please take another look.

[Ken Russell (switch to Gerrit), 2015-12-09 00:05:49]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-12-09 00:05:50]

This is a great upgrade; not an owner, but a couple of suggestions.

https://codereview.chromium.org/1508953002/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils.h (right):

https://codereview.chromium.org/1508953002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.h:138: // Optionally the skipped bytes
in the beginning can be returned.
How are they optionally returned? All of the outgoing arguments appear to be
mandatory. Either the documentation should be updated or the arguments named
something like opt_skip_size.

Also, consider avoiding overloading ComputeImageDataSizes. Perhaps this one
should be ComputeImageDataSizesES3, or ideally, they could be merged.

https://codereview.chromium.org/1508953002/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils_unittest.cc (right):

https://codereview.chromium.org/1508953002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils_unittest.cc:370: {  // skip_pixels,
skip_rows, skip_images
Perhaps add another test that also changes the pack alignment to 8 (from the
default of 4) and checks RGBA data with an odd number of pixels, or similar?

[Zhenyao Mo, 2015-12-09 00:27:57]

All revised.  Added a bunch more tests.  I think this version should be good.

Please take another look.

https://codereview.chromium.org/1508953002/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils.h (right):

https://codereview.chromium.org/1508953002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils.h:138: // Optionally the skipped bytes
in the beginning can be returned.
On 2015/12/09 00:05:50, Ken Russell wrote:
> How are they optionally returned? All of the outgoing arguments appear to be
> mandatory. Either the documentation should be updated or the arguments named
> something like opt_skip_size.

They are optional in the sense they can be nullptr.  Switch name to opt*

> 
> Also, consider avoiding overloading ComputeImageDataSizes. Perhaps this one
> should be ComputeImageDataSizesES3, or ideally, they could be merged.

Done

https://codereview.chromium.org/1508953002/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_utils_unittest.cc (right):

https://codereview.chromium.org/1508953002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_utils_unittest.cc:370: {  // skip_pixels,
skip_rows, skip_images
On 2015/12/09 00:05:50, Ken Russell wrote:
> Perhaps add another test that also changes the pack alignment to 8 (from the
> default of 4) and checks RGBA data with an odd number of pixels, or similar?

Done.

[Zhenyao Mo, 2015-12-09 00:28:14]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 00:31:17]

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

[commit-bot: I haz the power, 2015-12-09 01:57:50]

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

[commit-bot: I haz the power, 2015-12-09 01:57:51]

Dry run: This issue passed the CQ dry run.

[piman, 2015-12-09 02:08:16]

LGTM. I think it would read better with CheckedNumerics rather than the
SafeMultiply/Add, but this looks correct.

[Zhenyao Mo, 2015-12-09 17:05:17]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2015-12-09 17:07:25]

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

[commit-bot: I haz the power, 2015-12-09 17:16:10]

Description was changed from

==========
Implement helper functionalities for computing image size with ES3 settings.

This will be used in ReadPixels, Tex{Sub}Image* in followup CLs.

BUG=429053,563714
TEST=gpu_unittests
R=piman@chromium.org
==========

to

==========
Implement helper functionalities for computing image size with ES3 settings.

This will be used in ReadPixels, Tex{Sub}Image* in followup CLs.

BUG=429053,563714
TEST=gpu_unittests
R=piman@chromium.org
==========

[commit-bot: I haz the power, 2015-12-09 17:16:11]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-12-09 17:17:41]

Description was changed from

==========
Implement helper functionalities for computing image size with ES3 settings.

This will be used in ReadPixels, Tex{Sub}Image* in followup CLs.

BUG=429053,563714
TEST=gpu_unittests
R=piman@chromium.org
==========

to

==========
Implement helper functionalities for computing image size with ES3 settings.

This will be used in ReadPixels, Tex{Sub}Image* in followup CLs.

BUG=429053,563714
TEST=gpu_unittests
R=piman@chromium.org

Committed: https://crrev.com/e0b58d1f24bce6a6f19dcbe5e857e3731d0631ca
Cr-Commit-Position: refs/heads/master@{#364095}
==========

[commit-bot: I haz the power, 2015-12-09 17:17:42]

Patchset 6 (id:??) landed as
https://crrev.com/e0b58d1f24bce6a6f19dcbe5e857e3731d0631ca
Cr-Commit-Position: refs/heads/master@{#364095}

[Zhenyao Mo, 2015-12-09 19:08:54]

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/1511773006/ by zmo@chromium.org.

The reason for reverting is: Jamie thinks this caused the
GLES2ConformTest.GL2FixedTests_divide_by_zero_input_run test to fail on FYI
waterfall.