texture mipmap level is not zero-only in ES 3

texture mipmap level is not zero-only in ES 3

TEST=conformance2/renderbuffers/framebuffer-test.html, gpu_unittests

BUG=429053
Committed: https://crrev.com/9c63be8127c4c3a3d7f25d7e70d270499bb3c97f
Cr-Commit-Position: refs/heads/master@{#361159}

[yunchao, 2015-07-15 08:00:29]

yunchao.he@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, piman@chromium.org,
vmiura@chromium.org, zmo@chromium.org

[yunchao, 2015-07-15 08:14:52]

texture mipmap level is not zero only in ES 3. We should not translate
framebufferTexture2D(, , , , level) into framebufferTexture2D( , , , , 0). This
error is exposed by WebGL 2 conformance test. 

However, texture level is zero only in ES2. But I can't find a way to do the
check or not according to current rendering context version. So I think we
should remove the zero only limitation to texture mipmap level.

Any idea to do zero-only-check of mipmap-level only for an  ES2 rendering
context?

[Zhenyao Mo, 2015-07-15 17:01:02]

On 2015/07/15 08:14:52, yunchao wrote:
> texture mipmap level is not zero only in ES 3. We should not translate
> framebufferTexture2D(, , , , level) into framebufferTexture2D( , , , , 0).
This
> error is exposed by WebGL 2 conformance test. 
> 
> However, texture level is zero only in ES2. But I can't find a way to do the
> check or not according to current rendering context version. So I think we
> should remove the zero only limitation to texture mipmap level.
> 
> Any idea to do zero-only-check of mipmap-level only for an  ES2 rendering
> context?

Thanks for catching this bug.  I think it's OK to allow the commands pass
through on the client side with level > 0 in ES2.  So you don't need to modify
the code generator or write customized code.

You can do the ES2 && level == 0 check in
GLES2DecoderImpl::DoFramebufferTexture2DCommon()

Please add a test for the ES3 case.

[yunchao, 2015-07-21 10:43:56]

On 2015/07/15 17:01:02, Zhenyao Mo wrote:
> On 2015/07/15 08:14:52, yunchao wrote:
> > texture mipmap level is not zero only in ES 3. We should not translate
> > framebufferTexture2D(, , , , level) into framebufferTexture2D( , , , , 0).
> This
> > error is exposed by WebGL 2 conformance test. 
> > 
> > However, texture level is zero only in ES2. But I can't find a way to do the
> > check or not according to current rendering context version. So I think we
> > should remove the zero only limitation to texture mipmap level.
> > 
> > Any idea to do zero-only-check of mipmap-level only for an  ES2 rendering
> > context?
> 
> Thanks for catching this bug.  I think it's OK to allow the commands pass
> through on the client side with level > 0 in ES2.  So you don't need to modify
> the code generator or write customized code.
> 
> You can do the ES2 && level == 0 check in
> GLES2DecoderImpl::DoFramebufferTexture2DCommon()
> 
> Please add a test for the ES3 case.

Thanks for your reviewing, zmo@. Could you show me some examples to add test for
the ES3 case? Seems tha t unit tests in command buffer are auto-generated. There
have some unit tests for framebufferTexture2D with level > 0 for ES3. (Please
correct me if I am wrong. I am not familiar with command buffer currently).

[Zhenyao Mo, 2015-07-21 21:25:05]

https://codereview.chromium.org/1236053003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1236053003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1439: if (level > 0 &&
!feature_info_->IsES3Capable())
This is the wrong place for this validation.  We only want to validate level ==
0 in ES2 for glFramebufferTexture2D/Multisample, but ValidForTarget is for
TexImage2D() etc.  So basically you limit level to 0 for all texture related
operations.

The validation should go to GLES2DecoderImpl::DoFramebufferTexture2D().

Also, you should use IsES3Enabled(), not IsES3Capable().  We could still run ES2
context on top of ES3 drivers.

[Zhenyao Mo, 2015-07-21 21:36:47]

https://codereview.chromium.org/1236053003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h (right):

https://codereview.chromium.org/1236053003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h:921:
GL_TEXTURE_2D, kServiceTextureId, 5));
Actually these tests will fail, because level == 5 is passed in instead of 0.
More work needs to be put in to get the tests working.

To answer your other question about testing ES3, this is actually a good test
for the ES3 case. You only need to change GLES2DecoderTest1 to GLES3DecoderTest
and put it in gles2_cmd_decoder_unittest_framebuffers.cc

[yunchao, 2015-07-22 01:38:10]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-22 01:38:48]

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

[commit-bot: I haz the power, 2015-07-22 01:38:50]

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

[commit-bot: I haz the power, 2015-07-22 01:38:50]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[yunchao, 2015-11-10 07:01:18]

Description was changed from

==========
texture mipmap level is not zero-only in ES 3

BUG=429053
==========

to

==========
texture mipmap level is not zero-only in ES 3

TEST=conformance2/renderbuffers/framebuffer-test.html, gpu_unittests

BUG=429053
==========

[yunchao, 2015-11-10 07:04:38]

On 2015/07/21 21:36:47, Zhenyao Mo wrote:
>
https://codereview.chromium.org/1236053003/diff/20001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h
(right):
> 
>
https://codereview.chromium.org/1236053003/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h:921:
> GL_TEXTURE_2D, kServiceTextureId, 5));
> Actually these tests will fail, because level == 5 is passed in instead of 0.
> More work needs to be put in to get the tests working.
> 
> To answer your other question about testing ES3, this is actually a good test
> for the ES3 case. You only need to change GLES2DecoderTest1 to
GLES3DecoderTest
> and put it in gles2_cmd_decoder_unittest_framebuffers.cc

Zhenyao and all reviewers, I am very sorry that I have not updated this CL for
such a long time...  

Could you have a look when you have time? Thanks a lot!

[Ken Russell (switch to Gerrit), 2015-11-17 23:13:06]

Thanks for updating this. I'm not an OWNER and zmo@'s out of town right now but
the revised patch set lgtm. piman, vmiura, would either of you be able to help
re-review this?

[piman, 2015-11-17 23:40:06]

https://codereview.chromium.org/1236053003/diff/80001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1236053003/diff/80001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2011: 'ZeroOnlyForES2': {
What is the point of GLintZeroOnlyForES2 (as opposed to GLint)?

This seems like a fair amount of complexity in this file, where in the end this
seems to have no actual effect on the generated code, since the ES2 check is in
non-generated code.
The only thing I see is 1 autogenerated test, but it's a lot fewer lines of code
to just move that one to non-autogenerated tests.

[yunchao, 2015-11-19 10:14:11]

Thanks for your review, @piman.

https://codereview.chromium.org/1236053003/diff/80001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1236053003/diff/80001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2011: 'ZeroOnlyForES2': {
On 2015/11/17 23:40:06, piman (slow to review) wrote:
> What is the point of GLintZeroOnlyForES2 (as opposed to GLint)?
> 
> This seems like a fair amount of complexity in this file, where in the end
this
> seems to have no actual effect on the generated code, since the ES2 check is
in
> non-generated code.
> The only thing I see is 1 autogenerated test, but it's a lot fewer lines of
code
> to just move that one to non-autogenerated tests.

Yeah. The code in build_gles2_cmd_buffer.py seems to be a little complex, but
the autogenerated code is not that much. However, it also makes the mipmap level
to be 0 for autogenerated test cases for framebufferTexture2D and
framebufferTexture2DMultisample in gles2_implementation_unittest_autogen.h and
gles2_cmd_decoder_unittest_1_autogen.h. Otherwise, the mipmap level would be
autogenerated to 5, then the test cases would fail.

[zmo, 2015-11-19 19:15:00]

zmo@google.com changed reviewers:
+ zmo@google.com

[zmo, 2015-11-19 19:15:00]

Mostly look good.

https://codereview.chromium.org/1236053003/diff/80001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1236053003/diff/80001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2011: 'ZeroOnlyForES2': {
ZeroOnlyForES2 is misleading.  The code generator doesn't really generate codes
differently for ES2/ES3. The sole use of this is to generate 0 value for the ES2
tests.

Maybe it's better to remove this ZeroOnly thing as piman suggested, and move the
auto-generated tests to manual, so you can easily test the ES2 and ES3 cases
together (if not in one test, then a ES2 test followed by a ES3 test).

[yunchao, 2015-11-23 08:53:17]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 08:53:29]

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

[yunchao, 2015-11-23 10:44:39]

I have updated the code as you suggested: remove the ZeroOnly thing and move the
auto-generated tests to manual tests.

Please take a look.

https://codereview.chromium.org/1236053003/diff/80001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1236053003/diff/80001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2011: 'ZeroOnlyForES2': {
On 2015/11/19 19:15:00, zmo wrote:
> ZeroOnlyForES2 is misleading.  The code generator doesn't really generate
codes
> differently for ES2/ES3. The sole use of this is to generate 0 value for the
ES2
> tests.
> 
> Maybe it's better to remove this ZeroOnly thing as piman suggested, and move
the
> auto-generated tests to manual, so you can easily test the ES2 and ES3 cases
> together (if not in one test, then a ES2 test followed by a ES3 test).

Done.

[yunchao, 2015-11-23 10:52:46]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 10:53:00]

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

[commit-bot: I haz the power, 2015-11-23 12:16:59]

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

[commit-bot: I haz the power, 2015-11-23 12:16:59]

Dry run: This issue passed the CQ dry run.

[zmo, 2015-11-23 18:16:07]

LGTM

[piman, 2015-11-23 19:34:59]

lgtm

[zmo, 2015-11-23 19:41:01]

The CQ bit was checked by zmo@google.com

[zmo, 2015-11-23 19:41:01]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1236053003/#ps120001
(title: "code rebase")

[commit-bot: I haz the power, 2015-11-23 19:41:50]

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

[commit-bot: I haz the power, 2015-11-23 20:13:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-23 20:13:53]

Patchset 7 (id:??) landed as
https://crrev.com/9c63be8127c4c3a3d7f25d7e70d270499bb3c97f
Cr-Commit-Position: refs/heads/master@{#361159}