gles2_cmd_decoder_passthrough: Handle Tex[Sub]Image[2/3]D with empty shm

gles2_cmd_decoder_passthrough: Handle Tex[Sub]Image[2/3]D with empty shm

BUG=chromium:668223
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

Committed: https://crrev.com/178234d4c943ba92d9e8d7c9a6fb38daa9e19071
Cr-Commit-Position: refs/heads/master@{#439270}

[Corentin Wallez, 2016-12-16 22:32:36]

Description was changed from

==========
gles2_cmd_decoder_passthrough: Handle Tex[Sub]Image[2/3]D with empty shm

BUG=chromium:668223
==========

to

==========
gles2_cmd_decoder_passthrough: Handle Tex[Sub]Image[2/3]D with empty shm

BUG=chromium:668223
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
==========

[Corentin Wallez, 2016-12-16 22:33:31]

cwallez@chromium.org changed reviewers:
+ geofflang@chromium.org, zmo@chromium.org

[Corentin Wallez, 2016-12-16 22:33:32]

PTAL, this fixes a context lost when TexSubImage is called with width = height =
0.

[Corentin Wallez, 2016-12-16 22:33:35]

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

[commit-bot: I haz the power, 2016-12-16 22:34:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhenyao Mo, 2016-12-16 23:10:51]

On 2016/12/16 22:33:32, Corentin Wallez wrote:
> PTAL, this fixes a context lost when TexSubImage is called with width = height
=
> 0.

Can you explain to me why this CL fixes this border case?  From the code it's
not apparent to me.  Just try to understand.

[Corentin Wallez, 2016-12-16 23:49:37]

On 2016/12/16 at 23:10:51, zmo wrote:
> On 2016/12/16 22:33:32, Corentin Wallez wrote:
> > PTAL, this fixes a context lost when TexSubImage is called with width =
height =
> > 0.
> 
> Can you explain to me why this CL fixes this border case?  From the code it's
not apparent to me.  Just try to understand.

GLES2Implementation::TexSubImage2D special cases width = height = 0 and sends
(shm id = 0, offset = 0).
See
https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implemen...

[Zhenyao Mo, 2016-12-16 23:53:31]

On 2016/12/16 23:49:37, Corentin Wallez wrote:
> On 2016/12/16 at 23:10:51, zmo wrote:
> > On 2016/12/16 22:33:32, Corentin Wallez wrote:
> > > PTAL, this fixes a context lost when TexSubImage is called with width =
> height =
> > > 0.
> > 
> > Can you explain to me why this CL fixes this border case?  From the code
it's
> not apparent to me.  Just try to understand.
> 
> GLES2Implementation::TexSubImage2D special cases width = height = 0 and sends
> (shm id = 0, offset = 0).
> See
>
https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implemen...

I understand this.  I also understand why you want to assign the ID and offset
to local variables and use that instead, so we won't have inconsistent values
from the same param.

However, is that's being triggered (that's not an easy to trigger condition)?
Otherwise the code logic doesn't change.

[Corentin Wallez, 2016-12-17 00:07:07]

On 2016/12/16 at 23:53:31, zmo wrote:
> On 2016/12/16 23:49:37, Corentin Wallez wrote:
> > On 2016/12/16 at 23:10:51, zmo wrote:
> > > On 2016/12/16 22:33:32, Corentin Wallez wrote:
> > > > PTAL, this fixes a context lost when TexSubImage is called with width =
> > height =
> > > > 0.
> > > 
> > > Can you explain to me why this CL fixes this border case?  From the code
it's
> > not apparent to me.  Just try to understand.
> > 
> > GLES2Implementation::TexSubImage2D special cases width = height = 0 and
sends
> > (shm id = 0, offset = 0).
> > See
> >
https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implemen...
> 
> I understand this.  I also understand why you want to assign the ID and offset
to local variables and use that instead, so we won't have inconsistent values
from the same param.
> 
> However, is that's being triggered (that's not an easy to trigger condition)?
Otherwise the code logic doesn't change.

I'm not sure I understand the question, sorry. The initial reason I did this
change is that when width = height = 0, in HandleTexSubImage2D would return
error::kOutOfBounds which causes a context loss and makes the rest of the WebGL
CTS fail. Then I did the same fix for 3D and saw that HandleTexImage2D/3D read
the shm_id and offset twice and fixed that too.

[commit-bot: I haz the power, 2016-12-17 00:24:22]

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

[commit-bot: I haz the power, 2016-12-17 00:24:23]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2016-12-17 00:32:09]

OK, I missed the part where TexSubImage fixes are different from the TexImage
changes (that's me being lazy and less careful, sorry!)
LGTM

[Corentin Wallez, 2016-12-17 00:57:06]

On 2016/12/17 at 00:32:09, zmo wrote:
> OK, I missed the part where TexSubImage fixes are different from the TexImage
changes (that's me being lazy and less careful, sorry!)
> LGTM

Thanks for the review!

[Corentin Wallez, 2016-12-17 00:57:08]

The CQ bit was checked by cwallez@chromium.org

[commit-bot: I haz the power, 2016-12-17 00:57:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 01:06:01]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481936228439570, "parent_rev":
"ba6ef0af743f7405a97a2c9f5c452be6491331b8", "commit_rev":
"529ed2b04ed914493e1ab3accefc69c34e61b028"}

[commit-bot: I haz the power, 2016-12-17 01:06:29]

Description was changed from

==========
gles2_cmd_decoder_passthrough: Handle Tex[Sub]Image[2/3]D with empty shm

BUG=chromium:668223
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
==========

to

==========
gles2_cmd_decoder_passthrough: Handle Tex[Sub]Image[2/3]D with empty shm

BUG=chromium:668223
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/2587583002
==========

[commit-bot: I haz the power, 2016-12-17 01:06:32]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-17 01:08:50]

Description was changed from

==========
gles2_cmd_decoder_passthrough: Handle Tex[Sub]Image[2/3]D with empty shm

BUG=chromium:668223
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/2587583002
==========

to

==========
gles2_cmd_decoder_passthrough: Handle Tex[Sub]Image[2/3]D with empty shm

BUG=chromium:668223
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

Committed: https://crrev.com/178234d4c943ba92d9e8d7c9a6fb38daa9e19071
Cr-Commit-Position: refs/heads/master@{#439270}
==========

[commit-bot: I haz the power, 2016-12-17 01:08:51]

Patchset 1 (id:??) landed as
https://crrev.com/178234d4c943ba92d9e8d7c9a6fb38daa9e19071
Cr-Commit-Position: refs/heads/master@{#439270}