gpu: If not cube complete, the texture isn't renderable, no matter mipmap complete.

gpu: If not cube complete, the texture isn't renderable, no matter mipmap complete.

GLES2 spec 3.8.2 says
"A cube map sampler is called, and either the corresponding cube map texture
image is not cube complete, or TEXTURE_MIN_FILTER is one that requires
a mipmap and the texture is not mipmap cube complete."

However, Texture::GetCanRenderCondition() checks cube complete only if the texture
needs mipmap.

TEST=gpu_unittests TextureTest.POTCubeMapWithoutMipmap
BUG=517548
Committed: https://crrev.com/a326ed46c3923c0e3a29d030c10e3e76b3bdc288
Cr-Commit-Position: refs/heads/master@{#344456}

[dshwang, 2015-08-17 16:09:04]

dongseong.hwang@intel.com changed reviewers:
+ piman@chromium.org, sievers@chromium.org

[dshwang, 2015-08-17 16:09:05]

sievers, piman: could you review? It doesn't change any logic, but just
renaming.

When I resolve https://codereview.chromium.org/1283303002/, I need to use
CanRender() as "attachment incomplete", but it's awkward, so I rename it first.

[dshwang, 2015-08-17 16:20:41]

https://codereview.chromium.org/1299683002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/texture_manager.h (left):

https://codereview.chromium.org/1299683002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/texture_manager.h:323: // See section 3.8.2 of the
GLES2 spec.
section 3.8.2 is "Shader Execution", which isn't related to this code.
https://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf

[Ken Russell (switch to Gerrit), 2015-08-17 21:32:27]

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

[Ken Russell (switch to Gerrit), 2015-08-17 21:32:27]

https://codereview.chromium.org/1299683002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/texture_manager.h (left):

https://codereview.chromium.org/1299683002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/texture_manager.h:323: // See section 3.8.2 of the
GLES2 spec.
On 2015/08/17 16:20:41, dshwang wrote:
> section 3.8.2 is "Shader Execution", which isn't related to this code.
> https://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf

On the contrary: it is related. In GLES2 a texture that's complete may still not
be renderable, and the renderability rules are described in section 3.8.2. In
particular, sampling a non-power-of-two texture will return (0, 0, 0, 1)
("non-renderable" by this code's definition) under some circumstances, though
that texture might still be complete according to the rules in section 3.7.10.

I have concerns that this CL is not the right direction to go.

[dshwang, 2015-08-18 07:18:16]

https://codereview.chromium.org/1299683002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/texture_manager.h (left):

https://codereview.chromium.org/1299683002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/texture_manager.h:323: // See section 3.8.2 of the
GLES2 spec.
On 2015/08/17 21:32:27, Ken Russell wrote:
> On 2015/08/17 16:20:41, dshwang wrote:
> > section 3.8.2 is "Shader Execution", which isn't related to this code.
> > https://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf
> 
> On the contrary: it is related. In GLES2 a texture that's complete may still
not
> be renderable, and the renderability rules are described in section 3.8.2. In
> particular, sampling a non-power-of-two texture will return (0, 0, 0, 1)
> ("non-renderable" by this code's definition) under some circumstances, though
> that texture might still be complete according to the rules in section 3.7.10.
> 
> I have concerns that this CL is not the right direction to go.

I see. Let me not change the name. mentioning section 3.8.2 is also correct.
thank you for explanation.

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

https://codereview.chromium.org/1299683002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:449: return CAN_RENDER_NEVER;
I change the CL's goal. Texture::GetCanRenderCondition() is wrong about cube
complete. This fixes it.

Without this change, the unittest TextureTest.POTCubeMapWithoutMipmap fails as
follows;

[ RUN      ] TextureTest.POTCubeMapWithoutMipmap
../../gpu/command_buffer/service/texture_manager_unittest.cc:864: Failure
Value of: manager_->CanRender(texture_ref_.get())
  Actual: true
Expected: false
../../gpu/command_buffer/service/texture_manager_unittest.cc:865: Failure
Value of: manager_->HaveUnrenderableTextures()
  Actual: false
Expected: true
../../gpu/command_buffer/service/texture_manager_unittest.cc:873: Failure
Value of: manager_->CanRender(texture_ref_.get())
  Actual: true
Expected: false
../../gpu/command_buffer/service/texture_manager_unittest.cc:874: Failure
Value of: manager_->HaveUnrenderableTextures()
  Actual: false
Expected: true
../../gpu/command_buffer/service/texture_manager_unittest.cc:882: Failure
Value of: manager_->CanRender(texture_ref_.get())
  Actual: true
Expected: false
../../gpu/command_buffer/service/texture_manager_unittest.cc:883: Failure
Value of: manager_->HaveUnrenderableTextures()
  Actual: false
Expected: true
../../gpu/command_buffer/service/texture_manager_unittest.cc:890: Failure
Value of: manager_->CanRender(texture_ref_.get())
  Actual: true
Expected: false
../../gpu/command_buffer/service/texture_manager_unittest.cc:892: Failure
Value of: manager_->HaveUnrenderableTextures()
  Actual: false
Expected: true
../../gpu/command_buffer/service/texture_manager_unittest.cc:900: Failure
Value of: manager_->CanRender(texture_ref_.get())
  Actual: true
Expected: false
../../gpu/command_buffer/service/texture_manager_unittest.cc:901: Failure
Value of: manager_->HaveUnrenderableTextures()
  Actual: false
Expected: true
[  FAILED  ] TextureTest.POTCubeMapWithoutMipmap (1 ms)

[Ken Russell (switch to Gerrit), 2015-08-19 23:18:02]

Thanks. This seems OK to me but sievers or piman should review.

[piman, 2015-08-19 23:29:19]

lgtm

[dshwang, 2015-08-20 07:42:59]

On 2015/08/19 23:29:19, piman (slow to review) wrote:
> lgtm

thx for reviewing!

[dshwang, 2015-08-20 07:43:02]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2015-08-20 07:43:18]

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

[commit-bot: I haz the power, 2015-08-20 09:09:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-20 09:10:04]

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