CMAA: Avoid DCHECK failure for unsupported internalformats

CMAA: Avoid DCHECK failure for unsupported internalformats

CMAA implementation replies on CopyTextureCHROMIUM extension which only
supports part of ES3 formats. Skip CMAA for those unsupported formats.

BUG=535198, 687180
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/2697503002
Cr-Commit-Position: refs/heads/master@{#450920}
Committed: https://chromium.googlesource.com/chromium/src/+/d8d862a9d32dca327a856d7f01e7b300ff4105b0

[qiankun, 2017-02-13 08:22:03]

Description was changed from

==========
CMAA: Avoid DCHECK failure for unsupported internalformats

CMAA implementation replies on CopyTextureCHROMIUM extension which only
supports part of ES3 formats. Skip CMAA for those unsupported formats.

BUG=535198, 687180
==========

to

==========
CMAA: Avoid DCHECK failure for unsupported internalformats

CMAA implementation replies on CopyTextureCHROMIUM extension which only
supports part of ES3 formats. Skip CMAA for those unsupported formats.

BUG=535198, 687180
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
==========

[qiankun, 2017-02-13 08:52:24]

qiankun.miao@intel.com changed reviewers:
+ dongseong.hwang@intel.com, kbr@chromium.org, yunchao.he@intel.com,
zmo@chromium.org

[qiankun, 2017-02-13 08:52:25]

Bug https://bugs.chromium.org/p/chromium/issues/detail?id=687180 still existed
after https://codereview.chromium.org/2680703003 landed. According dongseong's
comments in https://codereview.chromium.org/2680703003#msg14, CMAA is only used
for RGB, RGBA, BGRA,
SRGB formats. I don't know why libfuzzer_chrome_asan_debug could triggerr the
failure. Skip CMAA for unsupported internalformats to see if the bug can be
fixed.

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/service/...
File
gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc
(right):

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:299:
DLOG(ERROR) << "Apply CMAA on framebuffer with attachment in "
I am not sure if the format limitation should be handled as a gl error, for
example, INVALID_OPERATION. There is no such format limitation in the extension
spec:
https://www.khronos.org/registry/OpenGL/extensions/INTEL/INTEL_framebuffer_CM....
So, it may be issue of this implementation in chrome. Dongseong, can you confirm
this?

[Zhenyao Mo, 2017-02-13 18:36:18]

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/service/...
File
gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc
(right):

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:299:
DLOG(ERROR) << "Apply CMAA on framebuffer with attachment in "
On 2017/02/13 08:52:25, qiankun wrote:
> I am not sure if the format limitation should be handled as a gl error, for
> example, INVALID_OPERATION. There is no such format limitation in the
extension
> spec:
>
https://www.khronos.org/registry/OpenGL/extensions/INTEL/INTEL_framebuffer_CM....
> So, it may be issue of this implementation in chrome. Dongseong, can you
confirm
> this?

I think for Chrome to generate an INVALID_OPERATION in such situation is fine -
much better than failing silently (DLOG usually isn't very useful)

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/tests/gl...
File
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc
(right):

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/tests/gl...
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc:198:
uint8_t pixels[1 * 4] = {0u, 255u, 0u, 255u};
Shouldn't this be part of the format_types structure? because if you ever expand
to other format_types, this data may not work.

Or, you can just remove the for loop and simply just test this specific
combination as is.

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/tests/gl...
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc:230:
uint8_t pixels[1 * 4] = {0u, 255u, 0u, 255u};
This looks wrong to me as it doesn't fit the format_types.  Same suggestion as
above.

[qiankun, 2017-02-14 08:08:17]

Thanks for reviewing.

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/service/...
File
gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc
(right):

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:299:
DLOG(ERROR) << "Apply CMAA on framebuffer with attachment in "
On 2017/02/13 18:36:17, Zhenyao Mo wrote:
> I think for Chrome to generate an INVALID_OPERATION in such situation is fine
-
> much better than failing silently (DLOG usually isn't very useful)

Generete INVALID_OPERATION for such invalid situation.

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/tests/gl...
File
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc
(right):

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/tests/gl...
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc:198:
uint8_t pixels[1 * 4] = {0u, 255u, 0u, 255u};
On 2017/02/13 18:36:18, Zhenyao Mo wrote:
> Shouldn't this be part of the format_types structure? because if you ever
expand
> to other format_types, this data may not work.
> 
> Or, you can just remove the for loop and simply just test this specific
> combination as is.

Done.

https://codereview.chromium.org/2697503002/diff/1/gpu/command_buffer/tests/gl...
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc:230:
uint8_t pixels[1 * 4] = {0u, 255u, 0u, 255u};
On 2017/02/13 18:36:18, Zhenyao Mo wrote:
> This looks wrong to me as it doesn't fit the format_types.  Same suggestion as
> above.

Done.

[Zhenyao Mo, 2017-02-15 18:22:12]

lgtm

[qiankun, 2017-02-16 05:30:33]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 05:31:28]

Dry run: 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-02-16 06:22:34]

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

[commit-bot: I haz the power, 2017-02-16 06:22:35]

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

[qiankun, 2017-02-16 09:52:18]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2017-02-16 09:53:03]

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-02-16 09:58:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487238738923260,
"parent_rev": "3c6d07134f1a93f57b6c0d191251deb3d9d9e887", "commit_rev":
"d8d862a9d32dca327a856d7f01e7b300ff4105b0"}

[commit-bot: I haz the power, 2017-02-16 09:59:10]

Description was changed from

==========
CMAA: Avoid DCHECK failure for unsupported internalformats

CMAA implementation replies on CopyTextureCHROMIUM extension which only
supports part of ES3 formats. Skip CMAA for those unsupported formats.

BUG=535198, 687180
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

==========
CMAA: Avoid DCHECK failure for unsupported internalformats

CMAA implementation replies on CopyTextureCHROMIUM extension which only
supports part of ES3 formats. Skip CMAA for those unsupported formats.

BUG=535198, 687180
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/2697503002
Cr-Commit-Position: refs/heads/master@{#450920}
Committed:
https://chromium.googlesource.com/chromium/src/+/d8d862a9d32dca327a856d7f01e7...
==========

[commit-bot: I haz the power, 2017-02-16 09:59:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d8d862a9d32dca327a856d7f01e7...