Fix fbo completeness check when non-base-level texture image is attached.

Fix fbo completeness check when non-base-level texture image is attached.

When non-base-level texture image is attached, it is required that texture is
mipmap complete for the fbo to be complete. Currently command buffer doesn't
check this.

BUG=722684
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2895493002
Cr-Commit-Position: refs/heads/master@{#473772}
Committed: https://chromium.googlesource.com/chromium/src/+/9585ee5ebcc8de435e2901b857901c736b6c1e40

[Zhenyao Mo, 2017-05-18 21:56:39]

Description was changed from

==========
Fix fbo completeness check when non-base-level texture image is attached.

When non-base-level texture image is attached, it is required that texture is
mipmap complete for the fbo to be complete. Currently command buffer doesn't
check this.

BUG=722684
TEST=gpu_unittests
R=piman@chromium.org
==========

to

==========
Fix fbo completeness check when non-base-level texture image is attached.

When non-base-level texture image is attached, it is required that texture is
mipmap complete for the fbo to be complete. Currently command buffer doesn't
check this.

BUG=722684
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[Zhenyao Mo, 2017-05-18 22:12:48]

piman: PTAL

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

https://codereview.chromium.org/2895493002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1485: if (texture_complete_ &&
texture_mips_dirty_) {
This original logic is wrong.  If at this point, texture_complete_ is true and
texture_mip_dirty_ is false, and after this if block, texture_complete_ is false
and texture_mip_dirty_ is cleared to true, then we are in a bad situation. 
Because now if you just call Update() again, when we come to this same place,
texture_complete_ is still true, but texture_mips_dirty_ has been cleared, so
this if block is bypassed, and texture_complete_ becomes true.  However, nothing
has changed between the two Update() calls. So it's apparently a bug (caught by
one of the tests)

[Zhenyao Mo, 2017-05-18 22:12:52]

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

[commit-bot: I haz the power, 2017-05-18 22:14:53]

Dry run: CQ is trying da patch.

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

[piman, 2017-05-18 22:50:52]

lgtm

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

https://codereview.chromium.org/2895493002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1443: return;
Are we guaranteed that if any of the fields/values used in this function is
modified, texture_mips_dirty_ will be set to true?

Maybe we should rename texture_mips_dirty_ into "completeness_dirty_" or
something.

[commit-bot: I haz the power, 2017-05-18 22:58:58]

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

[commit-bot: I haz the power, 2017-05-18 22:58:59]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[Zhenyao Mo, 2017-05-18 22:59:14]

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

https://codereview.chromium.org/2895493002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1443: return;
On 2017/05/18 22:50:52, piman wrote:
> Are we guaranteed that if any of the fields/values used in this function is
> modified, texture_mips_dirty_ will be set to true?
> 
> Maybe we should rename texture_mips_dirty_ into "completeness_dirty_" or
> something.

I think so.  The fields/values are only affected by SetLevelInfo(),
UpdateBaseLevel(), and UpdateMaxLevel(). All three sets the dirty bit.

I agree completeness_dirty_ is better. Will change to that.

[Zhenyao Mo, 2017-05-18 23:58:43]

piman: another bug fix. Please take a look again.

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

https://codereview.chromium.org/2895493002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1113: info.format != format ||
info.type != type || info.internal_workaround) {
This is another bug being exposed by this CL. I don't even know how it ever
worked for cubemap textures...

[Zhenyao Mo, 2017-05-18 23:58:49]

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

[commit-bot: I haz the power, 2017-05-18 23:59:49]

Dry run: CQ is trying da patch.

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

[piman, 2017-05-19 00:03:35]

lgtm

[piman, 2017-05-19 00:05:33]

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

https://codereview.chromium.org/2895493002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1192: info.internal_workaround =
true;
Oh, should we set completeness_dirty_ = true here?

[Zhenyao Mo, 2017-05-19 00:26:59]

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

https://codereview.chromium.org/2895493002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1192: info.internal_workaround =
true;
On 2017/05/19 00:05:33, piman wrote:
> Oh, should we set completeness_dirty_ = true here?

Done.

[Zhenyao Mo, 2017-05-19 00:27:04]

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

[commit-bot: I haz the power, 2017-05-19 00:28:09]

Dry run: CQ is trying da patch.

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

[piman, 2017-05-19 00:44:50]

lgtm

[commit-bot: I haz the power, 2017-05-19 01:20:03]

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

[commit-bot: I haz the power, 2017-05-19 01:20:04]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Zhenyao Mo, 2017-05-22 23:48:53]

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

[commit-bot: I haz the power, 2017-05-22 23:49:25]

Dry run: CQ is trying da patch.

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

[Zhenyao Mo, 2017-05-23 00:16:50]

piman: sorry to bug you again with this CL, but the WebGL conformance tests
caught another bug in our code.  Basically when we compute the num_mip_levels of
a texture face, we didn't take into consideration of the TexStorage case.

I think the latest patchset will pass all tests.

[piman, 2017-05-23 00:25:20]

lgtm

[commit-bot: I haz the power, 2017-05-23 01:00:49]

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

[commit-bot: I haz the power, 2017-05-23 01:00:50]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2017-05-23 01:09:54]

Description was changed from

==========
Fix fbo completeness check when non-base-level texture image is attached.

When non-base-level texture image is attached, it is required that texture is
mipmap complete for the fbo to be complete. Currently command buffer doesn't
check this.

BUG=722684
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Fix fbo completeness check when non-base-level texture image is attached.

When non-base-level texture image is attached, it is required that texture is
mipmap complete for the fbo to be complete. Currently command buffer doesn't
check this.

BUG=722684
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[Zhenyao Mo, 2017-05-23 01:09:59]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2017-05-23 01:10:26]

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, 2017-05-23 01:15:22]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495501799748900,
"parent_rev": "df2eff4a9cd6e379198f2c7cebdf6a11ea7157c1", "commit_rev":
"9585ee5ebcc8de435e2901b857901c736b6c1e40"}

[commit-bot: I haz the power, 2017-05-23 01:15:33]

Description was changed from

==========
Fix fbo completeness check when non-base-level texture image is attached.

When non-base-level texture image is attached, it is required that texture is
mipmap complete for the fbo to be complete. Currently command buffer doesn't
check this.

BUG=722684
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Fix fbo completeness check when non-base-level texture image is attached.

When non-base-level texture image is attached, it is required that texture is
mipmap complete for the fbo to be complete. Currently command buffer doesn't
check this.

BUG=722684
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2895493002
Cr-Commit-Position: refs/heads/master@{#473772}
Committed:
https://chromium.googlesource.com/chromium/src/+/9585ee5ebcc8de435e2901b85790...
==========

[commit-bot: I haz the power, 2017-05-23 01:15:34]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9585ee5ebcc8de435e2901b85790...