Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479513002:
Reland of Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. (Closed)
DescriptionReland of Extend CopyTextureCHROMIUM to more ES 3.0 texture formats.
Add support for the following copying techniques:
1) Using CopyTexImage2D when the source is color-renderable.
2) Drawing a quad when the destination is color-renderable.
3) Drawing to an intermediate texture, and copying from that
intermediate texture to the destination, when neither is
color-renderable.
Add support for nearly all of the new ES 3.0 texture formats.
Follow-on work remains, including some scenarios described in the bug,
such as copying to faces of currently-incomplete cube maps.
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
Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161
Committed: https://crrev.com/d12b5bc5fbeaa7729873bb7274fe8841de432c22
Cr-Original-Commit-Position: refs/heads/master@{#439312}
Cr-Commit-Position: refs/heads/master@{#439701}
Patch Set 1 #Patch Set 2 : support CopySubTextureCHROMIUM #
Total comments: 4
Patch Set 3 : fix windows and mac bot #
Total comments: 2
Patch Set 4 : Compare color on certain number of channels in gl_tests #Patch Set 5 : Implement CopyTextureCHROMIUM extension (part 1) #
Total comments: 2
Patch Set 6 : fix windows bots #Patch Set 7 : rebase only #Patch Set 8 : full path #
Total comments: 2
Patch Set 9 : fix ES2 context #Patch Set 10 : Add full gl_tests #Patch Set 11 : fix bots #Patch Set 12 : fix bgra #Patch Set 13 : Add sRGB extension support #Patch Set 14 : add missing file for SRGB ext fixup #Patch Set 15 : rebase and minor fix for premultiply and un-premultiply alpha #
Total comments: 24
Patch Set 16 : fix comments from zmo #17 #
Total comments: 18
Patch Set 17 : rebase only #Patch Set 18 : fix comments#19 #
Total comments: 26
Patch Set 19 : rebase only #Patch Set 20 : fix comment#26 #Patch Set 21 : rebase only #Patch Set 22 : cache fragment shader and program #Patch Set 23 : fix compressed formats #Patch Set 24 : fix compressed formats #
Total comments: 2
Patch Set 25 : remove addressed TODO #Patch Set 26 : fix GL_TEXTURE_EXTERNAL_OES in ES3 context #Patch Set 27 : add vertex shader cache #Patch Set 28 : rebase only #Patch Set 29 : fix-opengl-lessthan-32 #Messages
Total messages: 78 (30 generated)
Description was changed from ========== Implement CopyTextureCHROMIUM extension (part 1) This part implements the path which can be handled by glCopyTexImage2D. BUG=612542 ========== to ========== Implement CopyTextureCHROMIUM extension (part 1) This part implements the path which can be handled by glCopyTexImage2D. 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
Please take a look at the path using glCopyTexImage2D. There are still some hacks in the code. And I need to refactor the gl_tests. But, I want you to give some comments before I move forward. https://codereview.chromium.org/2479513002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2479513002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:16078: } We should move those code to gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc eventually. Keep it here to simplify the current CL. https://codereview.chromium.org/2479513002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:16203: copy_texture_CHROMIUM_->DoCopyTexImage2D( We should move those code to gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc eventually. Keep it here is to simplify the current CL. https://codereview.chromium.org/2479513002/diff/20001/gpu/command_buffer/test... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2479513002/diff/20001/gpu/command_buffer/test... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:51: "}\n"; Vertex shader and Fragment shader need to be optimized. They should be generated dynamically according destination texture format. https://codereview.chromium.org/2479513002/diff/20001/gpu/command_buffer/test... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:164: TEST_P(GLCopyTextureCHROMIUMTest, R8ToR8) { I still need to refactor the test code to simplify adding new formats combination.
It looks fine in general, but I didn't exactly do a line by line review this time. Also, what do we do if the target format has more channels than src format? Say, we try to copy R8 to RG8, what do we do? Do we generate INVALID_OP? or we default G channel to a value (0 or 1)? https://codereview.chromium.org/2479513002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2479513002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:13703: GLenum read_format, GLenum read_type, std::string& err_msg) { This is against coding style: https://google.github.io/styleguide/cppguide.html#Reference_Arguments std::string* output_error_msg. https://codereview.chromium.org/2479513002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:16060: bool legacy_format_valid = ValidateCopyTextureCHROMIUMInternalFormats( I think you should expand this function to handle all formats, and return an enum which tells you which path to go down.
On 2016/11/03 22:55:35, Zhenyao Mo wrote: > It looks fine in general, but I didn't exactly do a line by line review this > time. > > Also, what do we do if the target format has more channels than src format? Say, > we try to copy R8 to RG8, what do we do? Do we generate INVALID_OP? or we > default G channel to a value (0 or 1)? > If source texture doesn't contain a superset of the component required by destination internalformat, I fill the R/G/B channels with 0 and A channel with 1 to follow gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt. There are other two situations we need to consider when components size of source texture and destination texture doesn't match: 1. Each channel of source texture has larger size than that of destination texture: e.g. RGBA32I -> RGB8I. 2. Each channel of destination texture has larger size than that of source texture: e.g. RGBA8I -> RGBA32I. Should we map the color space between source texture format and destination texture format, such as [-2^31, 2^31-1] <--> [-2^8, 2^8-1]. And how we do mapping between signed integer and unsigned integer, 0 to 0u or -128 to 0u?
On 2016/11/04 09:06:56, qiankun wrote: > On 2016/11/03 22:55:35, Zhenyao Mo wrote: > > It looks fine in general, but I didn't exactly do a line by line review this > > time. > > > > Also, what do we do if the target format has more channels than src format? > Say, > > we try to copy R8 to RG8, what do we do? Do we generate INVALID_OP? or we > > default G channel to a value (0 or 1)? > > > > If source texture doesn't contain a superset of the component required by > destination internalformat, I fill the R/G/B channels with 0 and A channel with > 1 to follow gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt. > > There are other two situations we need to consider when components size of > source texture and destination texture doesn't match: > 1. Each channel of source texture has larger size than that of destination > texture: e.g. RGBA32I -> RGB8I. > 2. Each channel of destination texture has larger size than that of source > texture: e.g. RGBA8I -> RGBA32I. > > Should we map the color space between source texture format and destination > texture format, such as [-2^31, 2^31-1] <--> [-2^8, 2^8-1]. And how we do > mapping between signed integer and unsigned integer, 0 to 0u or -128 to 0u? Another question: If source texture is both non-color-renderable and non-filterable, how to handle copying from it to another texture, e.g. GL_RGB8I? We can not use glCopyTexImage2D since source texture is non-color-renderable, nor use the draw path since it's non-filterable. What's the value of such formats? Am I missing something?
On 2016/11/04 13:12:47, qiankun wrote: > On 2016/11/04 09:06:56, qiankun wrote: > > On 2016/11/03 22:55:35, Zhenyao Mo wrote: > > > It looks fine in general, but I didn't exactly do a line by line review this > > > time. > > > > > > Also, what do we do if the target format has more channels than src format? > > Say, > > > we try to copy R8 to RG8, what do we do? Do we generate INVALID_OP? or we > > > default G channel to a value (0 or 1)? > > > > > > > If source texture doesn't contain a superset of the component required by > > destination internalformat, I fill the R/G/B channels with 0 and A channel > with > > 1 to follow gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt. > > > > There are other two situations we need to consider when components size of > > source texture and destination texture doesn't match: > > 1. Each channel of source texture has larger size than that of destination > > texture: e.g. RGBA32I -> RGB8I. > > 2. Each channel of destination texture has larger size than that of source > > texture: e.g. RGBA8I -> RGBA32I. > > > > Should we map the color space between source texture format and destination > > texture format, such as [-2^31, 2^31-1] <--> [-2^8, 2^8-1]. And how we do > > mapping between signed integer and unsigned integer, 0 to 0u or -128 to 0u? I think we only need to handle 8-bit source textures, at least for now, per https://cs.chromium.org/chromium/src/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_c... . The conversion rules when upgrading to a texture format with more bits per channel should follow the rules in section 3.7.6 "Texture objects" in the WebGL 2.0 specification: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.6 specifically the table underneath texImage2D taking TexImageSource. That's the restricted subset of formats allowed for uploading e.g. hardware accelerated videos, or canvases, to WebGL. (Someday soon it will be possible to allocate a canvas with a high-bit-depth back buffer -- junov@'s driving this work -- but for your immediate work it's OK to keep it simple.) I don't think *any* scale factors should be applied during this copy. So if copying from an RGBA8 to RGBA32F texture, the source sampler should generate a value between 0.0 and 1.0, and the same value should be written to the floating-point texture. Note that signed integer formats aren't supported in that table in the WebGL 2.0 spec. The CHROMIUM_copy_texture extension should be updated with the new rules you're defining at the time you're writing the new code. Could you please take care of this as well? > Another question: > If source texture is both non-color-renderable and non-filterable, how to handle > copying from it to another texture, e.g. GL_RGB8I? We can not use > glCopyTexImage2D since source texture is non-color-renderable, nor use the draw > path since it's non-filterable. What's the value of such formats? Am I missing > something? You can just use the GL_NEAREST filter mode for the source texture in this case. The draw path can be used.
I did a code refactor. I think it's almost ready for review. I uploaded a clean CL in patch set 4 and uploaded the new code in patch set 5. You can review the difference between patch set 5 and patch set 4. https://codereview.chromium.org/2479513002/diff/80001/gpu/command_buffer/test... File gpu/command_buffer/tests/gl_test_utils.h (right): https://codereview.chromium.org/2479513002/diff/80001/gpu/command_buffer/test... gpu/command_buffer/tests/gl_test_utils.h:68: int channel_count = 4); If chrome disallows argument with default value, I can add this change as a separate CL to keep this CL clean. All calls to CheckPixles should be updated.
the bots are still red. https://codereview.chromium.org/2479513002/diff/80001/gpu/command_buffer/test... File gpu/command_buffer/tests/gl_test_utils.h (right): https://codereview.chromium.org/2479513002/diff/80001/gpu/command_buffer/test... gpu/command_buffer/tests/gl_test_utils.h:68: int channel_count = 4); On 2016/11/09 15:21:39, qiankun wrote: > If chrome disallows argument with default value, I can add this change as a > separate CL to keep this CL clean. All calls to CheckPixles should be updated. No, default value is not allowed. We should not land a CL with a default value, even just for a short period of time.
Patchset #6 (id:100001) has been deleted
All bots are green now. Please help to review.
Description was changed from ========== Implement CopyTextureCHROMIUM extension (part 1) This part implements the path which can be handled by glCopyTexImage2D. 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 ========== Implement CopyTextureCHROMIUM extension 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 ==========
There are some failures on Windows d3d9. I will investigate it tomorrow. I didn't reproduce them on Windows Intel d3d11 today. All paths are ready now. I have tested these three paths manually. TODOs: (1) add gl_tests for more internalformats. (2) enable it for WebGL. Please help to review if you are free. Thanks.
Qiankun, I didn't do a complete review because there are still some amount of work and testing for this main feature upgrade. Since our branch point is this Thursday, I worry even we are able to land this right before branch point, we don't have enough time to bake it in Canary. (This chromium extension is used by Chrome's internal operation and unfortunately testing are not well covering, so historically we saw bugs sneaking into chrome while modifying this extension.) At this point, I suggest we keep working on this CL to a landable state, but let's land it right after the branch point. So initial WebGL2 launch will be without this GPU-GPU fast path, but still fully conformant, and the next release will have this major speedup. https://codereview.chromium.org/2479513002/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:103: source += std::string("#version 150\n"); This is incorrect. Now we have 1) desktop core profile (mac) 2) desktop compatibility profile (linux) 3) ES3 profile (ANGLE ES3 on Windows, Android ES3) 4) ES2 profile (ANGLE ES2 on Windows, Android ES2) I think missing handling 4) is why D3D9 fails. Also, we don't have Android bots with ES2, but we definitely don't want to fail that platform because some low end phone users might still be on it. I don't understand why your CL doesn't cause failures on Linux, because here you just put it to ES3 path, and that ES3 shader should not work there. Can you investigate it a bit and let me know why?
Also please run against android gpu bot.
Very sorry for late. I took a lot of time to make the tests pass on various platforms. But, there are still some TODOs listed in the code. Some workarounds are needed to support full formats list, such as luminance emulation for Mac OSX. Please review this command buffer part when you are free. I will continue to work on blink side effort to enable gpu-gpu path for DOM elements. https://codereview.chromium.org/2479513002/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:103: source += std::string("#version 150\n"); On 2016/11/14 20:04:50, Zhenyao Mo wrote: > This is incorrect. Now we have > 1) desktop core profile (mac) > 2) desktop compatibility profile (linux) > 3) ES3 profile (ANGLE ES3 on Windows, Android ES3) > 4) ES2 profile (ANGLE ES2 on Windows, Android ES2) > > I think missing handling 4) is why D3D9 fails. Also, we don't have Android bots > with ES2, but we definitely don't want to fail that platform because some low > end phone users might still be on it. > > I don't understand why your CL doesn't cause failures on Linux, because here you > just put it to ES3 path, and that ES3 shader should not work there. Can you > investigate it a bit and let me know why? I updated the shaders per underlying context. I think it's OK to use GLSL for core profile and compatibility profile, ESSL 1.0 for ES2, and ESSL 3.0 for ES3. I don't know why previous ES3 shaders succeed to compile on Linux. What I see is Linux desktop compatibility profile accepts ES3 shaders.
Mostly looks good, but I still have a few questions. (I will be on vacation next week, so please work with kbr on getting this landed.) https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/feature_info.h (right): https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/feature_info.h:206: bool texture_format_bgra8888_available_; This is the wrong place for these two flags. The above three indicate that they are available but *NOT* enabled (so WebGL can dynamically enable them) What you need is two flags to be part of feature_flags_ (and I think you already have one, so only need to add the ext_srgb https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:91: "#ifdef GL_ES\n" You can use LONG_STRING_CONST so the shader sources can be more readable. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:212: case GL_TEXTURE_RECTANGLE_ARB: This is not needed (and it's not es2 thing, it's needed for Mac compatibility profile). https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:284: // RGB9_E5? Definite a float format, say GL_RGBA16F or RGBA32F. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:536: glGenTextures(1, &adjusted_texture); Where is this texture deleted? https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:537: glBindTexture(dest_target, adjusted_texture); I assume we already have the code to recover any changes states? Could you double check that it is the case (if not, we really need to) https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:604: GLuint adjusted_texture = dest_id; = 0. = dest_id is really confusing. I think it's better to name it intermediate_texture or so. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:605: glGenTextures(1, &adjusted_texture); Where is this texture deleted? https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:745: decoder->GetFeatureInfo()->context_type() == CONTEXT_TYPE_OPENGLES3) { 1) Why this is needed? 2) We definitely use this in WEBGL2 contexts, not just ES3 contexts https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13750: if (read_format == 0) { Can we DCHECK(output_error_msg)? https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:16216: return; nit: {} around return when the condition is multi-lines https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:16459: return; Can you add a comment here that an INVALID_OPERATION is already generated by ValidateCopyTextureCHROMIUMInternalFormats?
Ken, I fixed zhenyao's comments. Please help to review this CL when you are free. I am working on enabling this in blink for WebGL now. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/feature_info.h (right): https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/feature_info.h:206: bool texture_format_bgra8888_available_; On 2016/11/19 00:42:32, Zhenyao Mo wrote: > This is the wrong place for these two flags. > > The above three indicate that they are available but *NOT* enabled (so WebGL can > dynamically enable them) > > What you need is two flags to be part of feature_flags_ (and I think you already > have one, so only need to add the ext_srgb Yes. Thanks for pointing this. Follow your suggestions. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:91: "#ifdef GL_ES\n" On 2016/11/19 00:42:32, Zhenyao Mo wrote: > You can use LONG_STRING_CONST so the shader sources can be more readable. ifdef and define will be treated as C++ directives. In this format, we can print readable string to screen for debug. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:212: case GL_TEXTURE_RECTANGLE_ARB: On 2016/11/19 00:42:32, Zhenyao Mo wrote: > This is not needed (and it's not es2 thing, it's needed for Mac compatibility > profile). So, it's not available now since Mac uses core profile now. Do we still need to support GL_TEXTURE_RECTANGLE_ARB target? https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:284: // RGB9_E5? On 2016/11/19 00:42:32, Zhenyao Mo wrote: > Definite a float format, say GL_RGBA16F or RGBA32F. Use RGBA32F now. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:536: glGenTextures(1, &adjusted_texture); On 2016/11/19 00:42:33, Zhenyao Mo wrote: > Where is this texture deleted? Delete it now. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:537: glBindTexture(dest_target, adjusted_texture); On 2016/11/19 00:42:32, Zhenyao Mo wrote: > I assume we already have the code to recover any changes states? Could you > double check that it is the case (if not, we really need to) Both DoCopyTexImage2D and DoCopyTextureWithTransform path have code to rcover states: DoCopyTexImage2D: https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_cop.... DoCopyTextureWithTransform: https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_cop.... https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:604: GLuint adjusted_texture = dest_id; On 2016/11/19 00:42:33, Zhenyao Mo wrote: > = 0. = dest_id is really confusing. > > I think it's better to name it intermediate_texture or so. Done. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:605: glGenTextures(1, &adjusted_texture); On 2016/11/19 00:42:33, Zhenyao Mo wrote: > Where is this texture deleted? Done. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:745: decoder->GetFeatureInfo()->context_type() == CONTEXT_TYPE_OPENGLES3) { On 2016/11/19 00:42:32, Zhenyao Mo wrote: > 1) Why this is needed? This is used to avoid caching program and fragment shaders for ES3 and WebGL2. The current program and shader cache method doesn't consider sampler type and output type for ES3 and WebGL 2. > 2) We definitely use this in WEBGL2 contexts, not just ES3 contexts Added WEBGL2 context. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13750: if (read_format == 0) { On 2016/11/19 00:42:33, Zhenyao Mo wrote: > Can we DCHECK(output_error_msg)? Done. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:16216: return; On 2016/11/19 00:42:33, Zhenyao Mo wrote: > nit: {} around return when the condition is multi-lines Done. https://codereview.chromium.org/2479513002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:16459: return; On 2016/11/19 00:42:33, Zhenyao Mo wrote: > Can you add a comment here that an INVALID_OPERATION is already generated by > ValidateCopyTextureCHROMIUMInternalFormats? Done.
Mostly looks good. Tests need some extra work though. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:312: DLOG(ERROR) << "CopyTextureCHROMIUM: shader compilation failure." << log; nit: '.' -> ': ' https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:597: if (method == DRAW_AND_COPY) { This style is different from DoCopyTexture (which divides code into three blocks). Can we be consistent? https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:16037: // have bug on GL core profile. See crbug.com/577144. Enable the workaround It's not really a bug. something like "formats are not supported on GL core profile". https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:270: // RGB9_E5 isn't accepted by glCopyTexImage2D if underneath context is ES. This I don't understand. If it's not accepted in ES, shouldn't we go down a different path and still make it work? https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:417: continue; Again, use {} here and below. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:442: glDeleteFramebuffers(1, &framebuffer_id_); This is hard to understand. Can we not do it in the setup, so we don't have to do this here, instead, do it at the end of each loop? https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:512: glReadPixels(0, 0, kWidth, kHeight, GL_RGBA, GL_UNSIGNED_BYTE, This is wrong. You can't do this with certain formats.
kbr just reminded me that if is is cubemap in ES2, and if it is not cubemap complete, we will need to handle that situation (i.e., not go down the direct path, because it may not be renderable)
Patchset #18 (id:360001) has been deleted
https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:312: DLOG(ERROR) << "CopyTextureCHROMIUM: shader compilation failure." << log; On 2016/12/01 01:34:02, Zhenyao Mo wrote: > nit: '.' -> ': ' Done. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:597: if (method == DRAW_AND_COPY) { On 2016/12/01 01:34:02, Zhenyao Mo wrote: > This style is different from DoCopyTexture (which divides code into three > blocks). Can we be consistent? Done. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:16037: // have bug on GL core profile. See crbug.com/577144. Enable the workaround On 2016/12/01 01:34:02, Zhenyao Mo wrote: > It's not really a bug. something like "formats are not supported on GL core > profile". Done. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:270: // RGB9_E5 isn't accepted by glCopyTexImage2D if underneath context is ES. On 2016/12/01 01:34:02, Zhenyao Mo wrote: > This I don't understand. If it's not accepted in ES, shouldn't we go down a > different path and still make it work? I am afraid we cannot make RGB9_E5 work since it's not color-renderable nor supported by glCopyTexImage2D. glCopyTexImage2D requires component size of the source buffer and destination buffer to match. We even cannot find a proper intermediate format for RGB9_E5 in ES. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:417: continue; On 2016/12/01 01:34:02, Zhenyao Mo wrote: > Again, use {} here and below. Done. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:442: glDeleteFramebuffers(1, &framebuffer_id_); On 2016/12/01 01:34:02, Zhenyao Mo wrote: > This is hard to understand. Can we not do it in the setup, so we don't have to > do this here, instead, do it at the end of each loop? Done. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:512: glReadPixels(0, 0, kWidth, kHeight, GL_RGBA, GL_UNSIGNED_BYTE, On 2016/12/01 01:34:02, Zhenyao Mo wrote: > This is wrong. You can't do this with certain formats. The destination texture is drawn to a RGBA texture. See line482 CreateBackingForTexture().
On 2016/12/01 01:50:42, Zhenyao Mo wrote: > kbr just reminded me that if is is cubemap in ES2, and if it is not cubemap > complete, we will need to handle that situation (i.e., not go down the direct > path, because it may not be renderable) CopyTextureCHROMIUM doesn't support CUBE_MAP texture and requires level == 0, see gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt. This is why WebGLRenderingContextBase::texImageByGPU in Blink has a possibleDirectCopy path and a impossibleDirectCopy path. Should we allow CUBE_MAP target and level > 0 for CopyTextureCHROMIUM extension? With these two supports, it will simplify implementation of texImageByGPU in Blink.
I think this CL is already big enough, so LGTM as is (but wait for kbr to also take a look) TODO list: 1) add a readback path for RGB9_E5 and float formats (if extension isn't available and they are not color-renderable) 2) add support for uploading to cubemap textures and levels other than 0 3) Update extension doc to reflect this major upgrade - we probably will need a ES2 version and ES3 version https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:16079: valid_dest_format = feature_info_->ext_color_buffer_float_available(); Actually, even without this extension, so these format are not color-renderable, we should still make it work. It's just one path (more efficient if it's color-renderable) or another path (readback) https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:270: // RGB9_E5 isn't accepted by glCopyTexImage2D if underneath context is ES. On 2016/12/02 16:53:46, qiankun wrote: > On 2016/12/01 01:34:02, Zhenyao Mo wrote: > > This I don't understand. If it's not accepted in ES, shouldn't we go down a > > different path and still make it work? > > I am afraid we cannot make RGB9_E5 work since it's not color-renderable nor > supported by glCopyTexImage2D. glCopyTexImage2D requires component size of the > source buffer and destination buffer to match. We even cannot find a proper > intermediate format for RGB9_E5 in ES. So in ValidateCopyTextureCHROMIUMInternalFormats() you actually allowed this dest format. I think the extension should still support this through a slow readback path, i.e., reading back to CPU mem, and upload to the texture. This will allow us not to worry about it in the blink side. This can be done in a followup CL.
Description was changed from ========== Implement CopyTextureCHROMIUM extension 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 ========== Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 ==========
Very nice work Qiankun. Thank you for your persistence with this patch and sorry for the delay reviewing it. You and Mo have done great work together iterating on it. I have only one main concern, about the program cache key. Please read my comment below and tell me if you think that it is too large a change to include in the first version of this patch. We should not leave that performance regression in the source tree for very long, though. Please add a TODO in gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt about updating the extension's text once the work for this bug is done. I expanded upon the CL description, as it will show up in the commit history. Please rewrite it if you don't agree with my suggested edits. Thanks. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:103: source += std::string("#version 300 es\n"); All of the explicit constructor calls to std::string in this file seem unnecessary. The += operator works just as well with const char*. The extra code will be more difficult to maintain. Could you simplify it? https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:170: source += std::string("#define TextureType ivec4\n"); In particular: all of the explicit calls to std::string() here seem unnecessary. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:250: " TexCoordPrecision vec4 uv = u_tex_coord_transform * vec4(v_uv, 0, " I'd break this line after the "=" operator with a \n so that all the lines end in \n. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:253: " FRAGCOLOR = TextureType(color * InnerScaleValue) * " Same here. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:304: #ifndef NDEBUG Could you change this to #if DCHECK_IS_ON() ? We run release builds with dcheck_always_on=true, and this debugging code could be useful in that case. (DLOG works in this configuration.) https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:745: // TODO(qiankun.miao@intel.com): check if it is possible to cache program and It must be possible. The program cache key just needs to include all of the parameters that affect the shaders' source, including the source and destination formats. This is a big TODO. It will be a large slowdown for ES3 contexts, since it will recompile the shaders and relink the program every time Copy{Sub}TextureCHROMIUM is called. (Currently, at least if the application uses TexImage2D to upload GPU-accelerated HTML elements to WebGL, it gets the full speedup that CopyTextureCHROMIUM provides.) I think we should aim to solve this problem in the first iteration of this work, rather than leave it as a TODO. Could you expand out the ProgramMapKey to have a different namespace for ES2 and ES3 contexts, and for ES3 contexts, take into account the source and destination formats? I don't think it will be that difficult. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:21: enum SupportedCopyMethodByFormat { Could we choose another name for this enum? Perhaps CopyTextureMethod? https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:22: DIRECT_COPY, Could you please add a one-line comment documenting each of these? Mentioning that this uses CopyTex{Sub}Image2D to copy from the source to the destination, for example? https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:23: DIRECT_DRAW, Could you mention that this draws from the source to the destination texture? https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:24: DRAW_AND_COPY, Could you mention that this draws to an intermediate texture, and the copies to the destination texture? https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:25: NONE_COPY Perhaps NOT_COPYABLE? https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:83: uint8_t adjust_color[4]; adjust_color -> adjusted_color https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:272: // RGB9_E5 isn't accepted by glCopyTexImage2D if underneath context is ES. Please add a TODO here; one way or another we will need to solve this problem, either inside the CHROMIUM_copy_texture extension, or in the calling code. Also: underneath -> underlying.
Patchset #25 (id:520001) has been deleted
Patchset #24 (id:500001) has been deleted
Any update on this? We would really like to get it landed. The program cache key just needs to be expanded a little.
Please review Patch Set 20 for fix-up except program cache. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:16079: valid_dest_format = feature_info_->ext_color_buffer_float_available(); On 2016/12/03 00:39:20, Zhenyao Mo wrote: > Actually, even without this extension, so these format are not color-renderable, > we should still make it work. It's just one path (more efficient if it's > color-renderable) or another path (readback) Add a TODO for this. https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2479513002/diff/320001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:270: // RGB9_E5 isn't accepted by glCopyTexImage2D if underneath context is ES. On 2016/12/03 00:39:20, Zhenyao Mo wrote: > On 2016/12/02 16:53:46, qiankun wrote: > > On 2016/12/01 01:34:02, Zhenyao Mo wrote: > > > This I don't understand. If it's not accepted in ES, shouldn't we go down a > > > different path and still make it work? > > > > I am afraid we cannot make RGB9_E5 work since it's not color-renderable nor > > supported by glCopyTexImage2D. glCopyTexImage2D requires component size of the > > source buffer and destination buffer to match. We even cannot find a proper > > intermediate format for RGB9_E5 in ES. > > So in ValidateCopyTextureCHROMIUMInternalFormats() you actually allowed this > dest format. > > I think the extension should still support this through a slow readback path, > i.e., reading back to CPU mem, and upload to the texture. This will allow us not > to worry about it in the blink side. > > This can be done in a followup CL. Disable RGB9_E5 format for ES context before the readback path is implemented. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:103: source += std::string("#version 300 es\n"); On 2016/12/07 06:38:01, Ken Russell wrote: > All of the explicit constructor calls to std::string in this file seem > unnecessary. The += operator works just as well with const char*. The extra code > will be more difficult to maintain. Could you simplify it? Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:170: source += std::string("#define TextureType ivec4\n"); On 2016/12/07 06:38:01, Ken Russell wrote: > In particular: all of the explicit calls to std::string() here seem unnecessary. Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:250: " TexCoordPrecision vec4 uv = u_tex_coord_transform * vec4(v_uv, 0, " On 2016/12/07 06:38:01, Ken Russell wrote: > I'd break this line after the "=" operator with a \n so that all the lines end > in \n. Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:253: " FRAGCOLOR = TextureType(color * InnerScaleValue) * " On 2016/12/07 06:38:01, Ken Russell wrote: > Same here. Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:304: #ifndef NDEBUG On 2016/12/07 06:38:01, Ken Russell wrote: > Could you change this to #if DCHECK_IS_ON() ? We run release builds with > dcheck_always_on=true, and this debugging code could be useful in that case. > (DLOG works in this configuration.) Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:21: enum SupportedCopyMethodByFormat { On 2016/12/07 06:38:01, Ken Russell wrote: > Could we choose another name for this enum? Perhaps CopyTextureMethod? Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:22: DIRECT_COPY, On 2016/12/07 06:38:02, Ken Russell wrote: > Could you please add a one-line comment documenting each of these? Mentioning > that this uses CopyTex{Sub}Image2D to copy from the source to the destination, > for example? Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:23: DIRECT_DRAW, On 2016/12/07 06:38:02, Ken Russell wrote: > Could you mention that this draws from the source to the destination texture? Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:24: DRAW_AND_COPY, On 2016/12/07 06:38:01, Ken Russell wrote: > Could you mention that this draws to an intermediate texture, and the copies to > the destination texture? Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:25: NONE_COPY On 2016/12/07 06:38:02, Ken Russell wrote: > Perhaps NOT_COPYABLE? Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:83: uint8_t adjust_color[4]; On 2016/12/07 06:38:02, Ken Russell wrote: > adjust_color -> adjusted_color Done. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:272: // RGB9_E5 isn't accepted by glCopyTexImage2D if underneath context is ES. On 2016/12/07 06:38:02, Ken Russell wrote: > Please add a TODO here; one way or another we will need to solve this problem, > either inside the CHROMIUM_copy_texture extension, or in the calling code. Also: > underneath -> underlying. Done.
On 2016/12/09 19:26:50, Ken Russell wrote: > Any update on this? We would really like to get it landed. The program cache key > just needs to be expanded a little. Please review Patch Set 22 and Patch Set 23 for program cache. I debugged android bot failure in Patch Set 23 these two days. But I didn't find anything wrong. You can see there isn't any changes between Patch set 23 and Patch Set 24. But PS#23 failed many times on android gpu bot, while PS#24 passed the bot. https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:745: // TODO(qiankun.miao@intel.com): check if it is possible to cache program and On 2016/12/07 06:38:01, Ken Russell wrote: > It must be possible. The program cache key just needs to include all of the > parameters that affect the shaders' source, including the source and destination > formats. > > This is a big TODO. It will be a large slowdown for ES3 contexts, since it will > recompile the shaders and relink the program every time Copy{Sub}TextureCHROMIUM > is called. (Currently, at least if the application uses TexImage2D to upload > GPU-accelerated HTML elements to WebGL, it gets the full speedup that > CopyTextureCHROMIUM provides.) I think we should aim to solve this problem in > the first iteration of this work, rather than leave it as a TODO. > > Could you expand out the ProgramMapKey to have a different namespace for ES2 and > ES3 contexts, and for ES3 contexts, take into account the source and destination > formats? I don't think it will be that difficult. I extended fragment id to support source format and dest format. I think we cannot cache program by extending ProgramMapKey only, since we also need to cache fragment shader.
Maybe the android bot failure is a flaky bug. My another ANGLE CL (https://chromium-review.googlesource.com/#/c/417169/) also failed on android bot, see https://build.chromium.org/p/tryserver.chromium.angle/builders/android_angle_.... But that CL is a workaround which isn't enabled in chromium.
On 2016/12/10 00:34:25, qiankun wrote: > Maybe the android bot failure is a flaky bug. My another ANGLE CL > (https://chromium-review.googlesource.com/#/c/417169/) also failed on android > bot, see > https://build.chromium.org/p/tryserver.chromium.angle/builders/android_angle_.... > But that CL is a workaround which isn't enabled in chromium. Yes, there have been widespread Android test flakes recently. See http://crbug.com/672382 and http://crbug.com/672502 .
Excellent. Sorry for the delay re-reviewing. The expansion of the FragmentShaderId looks good. LGTM Please watch the Android bots on https://build.chromium.org/p/chromium.gpu.fyi/console as this lands, and revert if the WebGL conformance tests start failing. Unfortunately, we've lost all coverage on android_optional_gpu_tests_rel until http://crbug.com/672382 (and dependent bug http://crbug.com/672502 ) are fixed. https://codereview.chromium.org/2479513002/diff/540001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/540001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:920: // TODO(qiankun.miao@intel.com): check if it is possible to cache program and Remove TODO since it's been addressed.
Thanks for reviewing. I will monitor the android bot. https://codereview.chromium.org/2479513002/diff/540001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2479513002/diff/540001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:920: // TODO(qiankun.miao@intel.com): check if it is possible to cache program and On 2016/12/14 01:07:09, Ken Russell wrote: > Remove TODO since it's been addressed. Done. Thanks for reminding.
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, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2479513002/#ps560001 (title: "remove addressed TODO")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/12/14 11:44:13, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) I'm afraid that that test failure looks legitimate. It looks like a texture copy is failing somewhere and that the test's getting a black frame instead of the contents of the video. These are two more failures of the same test on your patch: https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... Qiankun, do you have the ability to investigate this on your side? It indicates there could be a significant problem with this patch on real OpenGL ES implementations.
On 2016/12/14 14:07:18, Ken Russell OOO-till-Dec-12 wrote: > On 2016/12/14 11:44:13, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > I'm afraid that that test failure looks legitimate. It looks like a texture copy > is failing somewhere and that the test's getting a black frame instead of the > contents of the video. These are two more failures of the same test on your > patch: > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > Qiankun, do you have the ability to investigate this on your side? It indicates > there could be a significant problem with this patch on real OpenGL ES > implementations. I debugged the android failure today. I found the failure was because of fragment shader generated for draw quad in ES3 context. I will continue to fix it tomorrow.
On 2016/12/14 14:07:18, Ken Russell OOO-till-Dec-19 wrote: > On 2016/12/14 11:44:13, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > I'm afraid that that test failure looks legitimate. It looks like a texture copy > is failing somewhere and that the test's getting a black frame instead of the > contents of the video. These are two more failures of the same test on your > patch: > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > Qiankun, do you have the ability to investigate this on your side? It indicates > there could be a significant problem with this patch on real OpenGL ES > implementations. Ken & Zhenyao, I fixed the android failure, PTAL. GL_TEXTURE_EXTERNAL_OES is exposed in ES 2.0, so it's invalid in ES 3 context. If the source target is GL_TEXTURE_EXTERNAL_OES, ESSL 1.0 shader should be generated for drawing quad.
Patchset #27 (id:600001) has been deleted
On 2016/12/16 04:49:06, qiankun wrote: > On 2016/12/14 14:07:18, Ken Russell OOO-till-Dec-19 wrote: > > On 2016/12/14 11:44:13, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > > > I'm afraid that that test failure looks legitimate. It looks like a texture > copy > > is failing somewhere and that the test's getting a black frame instead of the > > contents of the video. These are two more failures of the same test on your > > patch: > > > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > > > Qiankun, do you have the ability to investigate this on your side? It > indicates > > there could be a significant problem with this patch on real OpenGL ES > > implementations. > > Ken & Zhenyao, I fixed the android failure, PTAL. > GL_TEXTURE_EXTERNAL_OES is exposed in ES 2.0, so it's invalid in ES 3 context. > If the source target is GL_TEXTURE_EXTERNAL_OES, ESSL 1.0 shader should be > generated for drawing quad. Sorry, I don't understand this. If it's invalid in ES3 context, then if the underlying driver is ES3, we basically should never use GL_TEXTURE_EXTERNAL_OES. Are you actually saying that even in ES3, as far as we use ESSL 1.0 shader with TEXTURE_EXTERNAL_OES target, we are still OK?
> Are you actually saying that even in ES3, as far as we use ESSL 1.0 shader with > TEXTURE_EXTERNAL_OES target, we are still OK? That's right. Using ESSL 1.0 shader with TEXTURE_EXTERNAL_OES target in ES3 context is OK. This is the behavior before my CL.
On 2016/12/17 00:21:09, qiankun wrote: > > Are you actually saying that even in ES3, as far as we use ESSL 1.0 shader > with > > TEXTURE_EXTERNAL_OES target, we are still OK? > That's right. Using ESSL 1.0 shader with TEXTURE_EXTERNAL_OES target in ES3 > context is OK. > This is the behavior before my CL. lgtm
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.
The CQ bit was checked by qiankun.miao@intel.com
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/2479513002/#ps620001 (title: "add vertex shader cache")
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": 620001, "attempt_start_ts": 1481945863948360,
"parent_rev": "f56171f2c9aa6e64d6d36ec46daa7f7c45581800", "commit_rev":
"30763b962561054c41f549ffe2f44d2925efc338"}
Message was sent while issue was closed.
Description was changed from ========== Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 ========== Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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/2479513002 ==========
Message was sent while issue was closed.
Committed patchset #27 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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/2479513002 ========== to ========== Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312}
Message was sent while issue was closed.
On 2016/12/17 00:21:09, qiankun wrote: > > Are you actually saying that even in ES3, as far as we use ESSL 1.0 shader > with > > TEXTURE_EXTERNAL_OES target, we are still OK? > That's right. Using ESSL 1.0 shader with TEXTURE_EXTERNAL_OES target in ES3 > context is OK. > This is the behavior before my CL. Good work tracking this down Qiankun. We should probably inform the other graphics developers on Chromium of this, since multiple groups are using TEXTURE_EXTERNAL_OES type textures. I would suggest emailing graphics-dev@chromium.org about this finding, pointing to this CL. Do you think this might just be a bug in the driver? Thanks for pushing this work through! It's a tremendous improvement. Eagerly looking forward to the fixes to cube map texture uploads, which will fix a lot of currently broken WebGL conformance tests on Android.
Message was sent while issue was closed.
On 2016/12/18 03:23:00, Ken Russell OOO-till-Dec-19 wrote: > On 2016/12/17 00:21:09, qiankun wrote: > > > Are you actually saying that even in ES3, as far as we use ESSL 1.0 shader > > with > > > TEXTURE_EXTERNAL_OES target, we are still OK? > > That's right. Using ESSL 1.0 shader with TEXTURE_EXTERNAL_OES target in ES3 > > context is OK. > > This is the behavior before my CL. > > Good work tracking this down Qiankun. We should probably inform the other > graphics developers on Chromium of this, since multiple groups are using > TEXTURE_EXTERNAL_OES type textures. I would suggest emailing > mailto:graphics-dev@chromium.org about this finding, pointing to this CL. Do you think > this might just be a bug in the driver? See the extension spec: https://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt. The spec is written based on OpenGL ES 1.x and OpenGL ES 2.x. Do you think should TEXTURE_EXTERNAL_OES be supported if the underlying driver is ES3? > Thanks for pushing this work through! It's a tremendous improvement. Eagerly > looking forward to the fixes to cube map texture uploads, which will fix a lot > of currently broken WebGL conformance tests on Android.
Message was sent while issue was closed.
A revert of this CL (patchset #27 id:620001) has been created in https://codereview.chromium.org/2589613002/ by tkent@chromium.org. The reason for reverting is: Caused crashes in dozens of layout tests. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty... .
Message was sent while issue was closed.
Description was changed from ========== Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312} ========== to ========== Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312} ==========
Description was changed from ========== Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312} ========== to ========== Reland of Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312} ==========
Please take another look. I updated the failure reason at https://bugs.chromium.org/p/chromium/issues/detail?id=612542#c22. Can I run the chromium_webkit bot before landing? I didn't find it in the bots.
On 2016/12/19 08:56:38, qiankun wrote: > Please take another look. I updated the failure reason at > https://bugs.chromium.org/p/chromium/issues/detail?id=612542#c22. > Can I run the chromium_webkit bot before landing? I didn't find it in the bots. You can try with the blink bots (I already added them). The changes LGTM, but let's wait and see what the blink bots say
The CQ bit was checked by qiankun.miao@intel.com
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/2479513002/#ps660001 (title: "fix-opengl-lessthan-32")
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 qiankun.miao@intel.com
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...
CQ is committing da patch.
Bot data: {"patchset_id": 660001, "attempt_start_ts": 1482206645470790,
"parent_rev": "925619041bf41a559c530b00e31f085a4c8a52c1", "commit_rev":
"a84703793408c21d09fd895dcf976c80feac1da9"}
Message was sent while issue was closed.
Description was changed from ========== Reland of Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312} ========== to ========== Reland of Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312} Review-Url: https://codereview.chromium.org/2479513002 ==========
Message was sent while issue was closed.
Committed patchset #29 (id:660001)
Message was sent while issue was closed.
Description was changed from ========== Reland of Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Cr-Commit-Position: refs/heads/master@{#439312} Review-Url: https://codereview.chromium.org/2479513002 ========== to ========== Reland of Extend CopyTextureCHROMIUM to more ES 3.0 texture formats. Add support for the following copying techniques: 1) Using CopyTexImage2D when the source is color-renderable. 2) Drawing a quad when the destination is color-renderable. 3) Drawing to an intermediate texture, and copying from that intermediate texture to the destination, when neither is color-renderable. Add support for nearly all of the new ES 3.0 texture formats. Follow-on work remains, including some scenarios described in the bug, such as copying to faces of currently-incomplete cube maps. 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 Committed: https://crrev.com/bcf132827ebd0436fd3620bbda4cbcf3febdc161 Committed: https://crrev.com/d12b5bc5fbeaa7729873bb7274fe8841de432c22 Cr-Original-Commit-Position: refs/heads/master@{#439312} Cr-Commit-Position: refs/heads/master@{#439701} ==========
Message was sent while issue was closed.
Patchset 29 (id:??) landed as https://crrev.com/d12b5bc5fbeaa7729873bb7274fe8841de432c22 Cr-Commit-Position: refs/heads/master@{#439701}
Message was sent while issue was closed.
Thanks Qiankun for tracking down the problem with the layout tests. LGTM after the fact. |
