Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686043002:
Refactor CopyTextureCHROMIUM internalformat validation (Closed)
DescriptionRefactor CopyTextureCHROMIUM internalformat validation
Move CopyTexture method decision to a common place.
BUG=612542
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/2686043002
Cr-Commit-Position: refs/heads/master@{#451560}
Committed: https://chromium.googlesource.com/chromium/src/+/7ff34467488f2eb5e4fa0dc38f14e18a4a17584f
Patch Set 1 #Patch Set 2 : rebase only #
Messages
Total messages: 25 (16 generated)
Description was changed from ========== Refactor CopyTextureCHROMIUM internalformat validation Move CopyTexture method decision to a common place. BUG=612542 ========== to ========== Refactor CopyTextureCHROMIUM internalformat validation Move CopyTexture method decision to a common place. BUG=612542 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.miao@intel.com changed reviewers: + kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org
PTAL.
Please help to review this refactoring code.
On 2017/02/13 08:54:20, qiankun wrote: > Please help to review this refactoring code. lgtm
Qiankun: a heads up that https://codereview.chromium.org/2680703003 may be reverted in http://crbug.com/691741 due to a performance regression. I'd encourage you to try to reproduce it in house. It looks like this change may have a dependency on the other one so we should resolve that other one first.
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/14 01:08:27, Ken Russell wrote: > Qiankun: a heads up that https://codereview.chromium.org/2680703003 may be > reverted in http://crbug.com/691741 due to a performance regression. I'd > encourage you to try to reproduce it in house. It looks like this change may > have a dependency on the other one so we should resolve that other one first. Performance regression has been fixed. Land this CL now.
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for gpu/command_buffer/service/gles2_cmd_decoder.cc:
While running git apply --index -p1;
error: patch failed: gpu/command_buffer/service/gles2_cmd_decoder.cc:2027
error: gpu/command_buffer/service/gles2_cmd_decoder.cc: patch does not apply
Patch: gpu/command_buffer/service/gles2_cmd_decoder.cc
Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc
b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index
355540cce25e6824e173983bd5f0c7d90da0826e..4c89e7fb9dd2c44434c39abd4948df1f71c7b341
100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -2027,8 +2027,11 @@ class GLES2DecoderImpl : public GLES2Decoder, public
ErrorStateClient {
TextureRef* dest_texture_ref);
CopyTextureMethod ValidateCopyTextureCHROMIUMInternalFormats(
const char* function_name,
- TextureRef* source_texture_ref,
GLint source_level,
+ GLenum source_internal_format,
+ GLenum source_type,
+ GLenum dest_target,
+ GLint dest_level,
GLenum dest_internal_format);
bool ValidateCompressedCopyTextureCHROMIUM(const char* function_name,
TextureRef* source_texture_ref,
@@ -16224,15 +16227,12 @@ bool
GLES2DecoderImpl::ValidateCopyTextureCHROMIUMTextures(
CopyTextureMethod GLES2DecoderImpl::ValidateCopyTextureCHROMIUMInternalFormats(
const char* function_name,
- TextureRef* source_texture_ref,
GLint source_level,
+ GLenum source_internal_format,
+ GLenum source_type,
+ GLenum dest_target,
+ GLint dest_level,
GLenum dest_internal_format) {
- GLenum source_type = 0;
- GLenum source_internal_format = 0;
- Texture* source_texture = source_texture_ref->texture();
- source_texture->GetLevelType(source_texture->target(), source_level,
- &source_type, &source_internal_format);
-
bool valid_dest_format = false;
// TODO(qiankun.miao@intel.com): ALPHA, LUMINANCE and LUMINANCE_ALPHA formats
// are not supported on GL core profile. See crbug.com/577144. Enable the
@@ -16326,12 +16326,26 @@ CopyTextureMethod
GLES2DecoderImpl::ValidateCopyTextureCHROMIUMInternalFormats(
dest_internal_format != GL_BGRA8_EXT &&
ValidateCopyTexFormatHelper(dest_internal_format, source_internal_format,
source_type, &output_error_msg);
- if (source_format_color_renderable && copy_tex_image_format_valid)
- return DIRECT_COPY;
- if (dest_format_color_renderable)
+ // TODO(qiankun.miao@intel.com): for WebGL 2.0 or OpenGL ES 3.0, both
+ // DIRECT_DRAW path for dest_level > 0 and DIRECT_COPY path for source_level
>
+ // 0 are not available due to a framebuffer completeness bug:
+ // crbug.com/678526. Once the bug is fixed, the limitation for WebGL 2.0 and
+ // OpenGL ES 3.0 can be lifted.
+ // For WebGL 1.0 or OpenGL ES 2.0, DIRECT_DRAW path isn't available for
+ // dest_level > 0 due to level > 0 isn't supported by glFramebufferTexture2D
+ // in ES2 context. DIRECT_DRAW path isn't available for cube map dest texture
+ // either due to it may be cube map incomplete. Go to DRAW_AND_COPY path in
+ // these cases.
+ if (source_format_color_renderable && copy_tex_image_format_valid &&
+ source_level == 0)
+ return DIRECT_COPY;
+ if (dest_format_color_renderable && dest_level == 0 &&
+ dest_target != GL_TEXTURE_CUBE_MAP)
return DIRECT_DRAW;
+ // Draw to a fbo attaching level 0 of an intermediate texture,
+ // then copy from the fbo to dest texture level with glCopyTexImage2D.
return DRAW_AND_COPY;
}
@@ -16429,31 +16443,14 @@ void GLES2DecoderImpl::DoCopyTextureCHROMIUM(
}
CopyTextureMethod method = ValidateCopyTextureCHROMIUMInternalFormats(
- kFunctionName, source_texture_ref, source_level, internal_format);
+ kFunctionName, source_level, source_internal_format, source_type,
+ dest_binding_target, dest_level, internal_format);
// INVALID_OPERATION is already generated by
// ValidateCopyTextureCHROMIUMInternalFormats.
if (method == NOT_COPYABLE) {
return;
}
- // Draw to a fbo attaching level 0 of an intermediate texture,
- // then copy from the fbo to dest texture level with glCopyTexImage2D.
- // For WebGL 1.0 or OpenGL ES 2.0, DIRECT_DRAW path isn't available for
- // dest_level > 0 due to level > 0 isn't supported by glFramebufferTexture2D
- // in ES2 context. DIRECT_DRAW path isn't available for cube map dest texture
- // either due to it may be cube map incomplete. Go to DRAW_AND_COPY path in
- // these cases.
- // TODO(qiankun.miao@intel.com): for WebGL 2.0 or OpenGL ES 3.0, both
- // DIRECT_DRAW path for dest_level > 0 and DIRECT_COPY path for source_level
>
- // 0 are not available due to a framebuffer completeness bug:
- // crbug.com/678526. Once the bug is fixed, the limitation for WebGL 2.0 and
- // OpenGL ES 3.0 can be lifted.
- if (((dest_level > 0 || dest_binding_target == GL_TEXTURE_CUBE_MAP) &&
- method == DIRECT_DRAW) ||
- (source_level > 0 && method == DIRECT_COPY)) {
- method = DRAW_AND_COPY;
- }
-
if (feature_info_->feature_flags().desktop_srgb_support) {
bool enable_framebuffer_srgb =
GLES2Util::GetColorEncodingFromInternalFormat(source_internal_format)
==
@@ -16698,7 +16695,8 @@ void GLES2DecoderImpl::DoCopySubTextureCHROMIUM(
}
CopyTextureMethod method = ValidateCopyTextureCHROMIUMInternalFormats(
- kFunctionName, source_texture_ref, source_level, dest_internal_format);
+ kFunctionName, source_level, source_internal_format, source_type,
+ dest_binding_target, dest_level, dest_internal_format);
// INVALID_OPERATION is already generated by
// ValidateCopyTextureCHROMIUMInternalFormats.
if (method == NOT_COPYABLE) {
@@ -16716,24 +16714,6 @@ void GLES2DecoderImpl::DoCopySubTextureCHROMIUM(
}
#endif
- // Draw to a fbo attaching level 0 of an intermediate texture,
- // then copy from the fbo to dest texture level with glCopyTexImage2D.
- // For WebGL 1.0 or OpenGL ES 2.0, DIRECT_DRAW path isn't available for
- // dest_level > 0 due to level > 0 isn't supported by glFramebufferTexture2D
- // in ES2 context. DIRECT_DRAW path isn't available for cube map dest texture
- // either due to it may be cube map incomplete. Go to DRAW_AND_COPY path in
- // these cases.
- // TODO(qiankun.miao@intel.com): for WebGL 2.0 or OpenGL ES 3.0, both
- // DIRECT_DRAW path for dest_level > 0 and DIRECT_COPY path for source_level
>
- // 0 are not available due to a framebuffer completeness bug:
- // crbug.com/678526. Once the bug is fixed, the limitation for WebGL 2.0 and
- // OpenGL ES 3.0 can be lifted.
- if (((dest_level > 0 || dest_binding_target == GL_TEXTURE_CUBE_MAP) &&
- method == DIRECT_DRAW) ||
- (source_level > 0 && method == DIRECT_COPY)) {
- method = DRAW_AND_COPY;
- }
-
if (feature_info_->feature_flags().desktop_srgb_support) {
bool enable_framebuffer_srgb =
GLES2Util::GetColorEncodingFromInternalFormat(source_internal_format)
==
The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2686043002/#ps20001 (title: "rebase only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487567850955980,
"parent_rev": "17312b794d9fa968a6cb09e2cf4eb0e9ecf743df", "commit_rev":
"7ff34467488f2eb5e4fa0dc38f14e18a4a17584f"}
Message was sent while issue was closed.
Description was changed from ========== Refactor CopyTextureCHROMIUM internalformat validation Move CopyTexture method decision to a common place. BUG=612542 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 ========== Refactor CopyTextureCHROMIUM internalformat validation Move CopyTexture method decision to a common place. BUG=612542 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/2686043002 Cr-Commit-Position: refs/heads/master@{#451560} Committed: https://chromium.googlesource.com/chromium/src/+/7ff34467488f2eb5e4fa0dc38f14... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7ff34467488f2eb5e4fa0dc38f14... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
