Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760843002:
Add readback path for CopyTextureCHROMIUM (Closed)
DescriptionAdd readback path for CopyTextureCHROMIUM
There are some liminations or driver bugs which cause GPU-GPU copy
unavailable in CopyTextureCHROMIUM. For such cases, we do a readback
fallback, i.e. draw the source texture to an intermediate texture in
RGBA format, read back pixels of the intermediate texture to a pbo,
then upload pixels to destination texture with TexImage api. With
this approach, the caller of glCopyTextureCHROMIUM doesn't need to care
about the liminations and bugs mentioned above.
BUG=612542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2760843002
Cr-Commit-Position: refs/heads/master@{#460994}
Committed: https://chromium.googlesource.com/chromium/src/+/986a2209658887e85eeb3e00c933db92383bef73
Patch Set 1 #
Total comments: 9
Patch Set 2 : rebase only #Patch Set 3 : use pack/unpack buffer #
Total comments: 16
Patch Set 4 : fix comments#20 #
Messages
Total messages: 32 (19 generated)
Description was changed from ========== Add readback path for CopyTextureCHROMIUM There are some liminations or driver bugs which cause GPU-GPU copy unavailable in CopyTextureCHROMIUM. For such cases, we do a readback fallback, i.e. draw the source texture to an intermediate texture in RGBA format, read back pixels of the intermediate texture from GPU to CPU, then upload pixels to destination texture with TexImage api. With this approach, the caller of glCopyTextureCHROMIUM doesn't need to care about the liminations and bugs mentioned above. BUG=612542 ========== to ========== Add readback path for CopyTextureCHROMIUM There are some liminations or driver bugs which cause GPU-GPU copy unavailable in CopyTextureCHROMIUM. For such cases, we do a readback fallback, i.e. draw the source texture to an intermediate texture in RGBA format, read back pixels of the intermediate texture from GPU to CPU, then upload pixels to destination texture with TexImage api. With this approach, the caller of glCopyTextureCHROMIUM doesn't need to care about the liminations and bugs mentioned above. BUG=612542 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
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...
qiankun.miao@intel.com changed reviewers: + kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org
PTAL. With this patch, we can enable CopyTextureCHROMIUM in blink without a log of webgl cts suppressions for driver bugs or limitation of the extension.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for keeping pushing forward. Very good work! A few suggestions for you to consider. https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:667: glReadPixels(0, 0, width, height, GL_RGBA, GL_UNSIGNED_BYTE, buf.get()); It would be really beneficial to explore the path of reading back to a PIXEL_PACK_BUFFER. Also, on desktop GL, you can directly read to a RGB/UNSIGNED_BYTE buffer or FLOAT buffer. On ES, you can probably use MapBufferRange to directly change PIXEL_PACK_BUFFER from RGBA to RGB; for floats, you probably can create a PIXEL_UNPACK_BUFFER and map it, and directly write to it. This could be a significant perf gain for these formats, but could you write some simple tests (repeatedly uploading textures to these formats) to measure the perf difference, so we have confidence that what we implement is optimal? https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16556: #if defined(OS_MACOSX) Can we hook this up with a driver bug workaround so we only limit it to NVidia on OSX? https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16569: // resolved. Can we not put them on DRAW_AND_READBACK path for now? So at least the performance will be good.
https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:667: glReadPixels(0, 0, width, height, GL_RGBA, GL_UNSIGNED_BYTE, buf.get()); On 2017/03/20 17:05:40, Zhenyao Mo wrote: > It would be really beneficial to explore the path of reading back to a > PIXEL_PACK_BUFFER. Also, on desktop GL, you can directly read to a > RGB/UNSIGNED_BYTE buffer or FLOAT buffer. On ES, you can probably use > MapBufferRange to directly change PIXEL_PACK_BUFFER from RGBA to RGB; for > floats, you probably can create a PIXEL_UNPACK_BUFFER and map it, and directly > write to it. This is a good suggestion. Thanks! I did experiment. This method works. > > This could be a significant perf gain for these formats, but could you write > some simple tests (repeatedly uploading textures to these formats) to measure > the perf difference, so we have confidence that what we implement is optimal? What kinds of test are in your mind, gl_tests or webgl tests? Should I add these tests into this CL or just do some perf comparison locally? What the perf difference should I measure, readback to client memory vs. readback to pbo? https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16556: #if defined(OS_MACOSX) On 2017/03/20 17:05:40, Zhenyao Mo wrote: > Can we hook this up with a driver bug workaround so we only limit it to NVidia > on OSX? I can do this. https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16569: // resolved. On 2017/03/20 17:05:40, Zhenyao Mo wrote: > Can we not put them on DRAW_AND_READBACK path for now? So at least the > performance will be good. We cannot detect internalformat in texSubImage2D in Blink. So, it's impossible to filter SRGB subImage uploading unless we disable CopyTextureCHROMIUM for all format/type combinations when doing texSubImage2D.
On 2017/03/21 08:47:13, qiankun wrote: > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): > > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:667: > glReadPixels(0, 0, width, height, GL_RGBA, GL_UNSIGNED_BYTE, buf.get()); > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > It would be really beneficial to explore the path of reading back to a > > PIXEL_PACK_BUFFER. Also, on desktop GL, you can directly read to a > > RGB/UNSIGNED_BYTE buffer or FLOAT buffer. On ES, you can probably use > > MapBufferRange to directly change PIXEL_PACK_BUFFER from RGBA to RGB; for > > floats, you probably can create a PIXEL_UNPACK_BUFFER and map it, and directly > > write to it. > This is a good suggestion. Thanks! I did experiment. This method works. > > > > > This could be a significant perf gain for these formats, but could you write > > some simple tests (repeatedly uploading textures to these formats) to measure > > the perf difference, so we have confidence that what we implement is optimal? > What kinds of test are in your mind, gl_tests or webgl tests? Should I add these > tests into this CL or just do some perf comparison locally? What the perf > difference should I measure, readback to client memory vs. readback to pbo? Maybe some tests to put in the WebGL github repo, so others can also use it? Maybe under src/sdk/performance/textures/? > > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:16556: #if defined(OS_MACOSX) > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > Can we hook this up with a driver bug workaround so we only limit it to NVidia > > on OSX? > > I can do this. > > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:16569: // resolved. > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > Can we not put them on DRAW_AND_READBACK path for now? So at least the > > performance will be good. > > We cannot detect internalformat in texSubImage2D in Blink. So, it's impossible > to filter SRGB subImage uploading unless we disable CopyTextureCHROMIUM for all > format/type combinations when doing texSubImage2D. SRGB8 is not color-renderable, but SRGB8_ALPHA8 is, so why SRGB8_ALPHA8 needs to be on a different path than, say, RGBA8?
https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16569: // resolved. On 2017/03/21 08:47:13, qiankun wrote: > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > Can we not put them on DRAW_AND_READBACK path for now? So at least the > > performance will be good. > > > We cannot detect internalformat in texSubImage2D in Blink. So, it's impossible > > to filter SRGB subImage uploading unless we disable CopyTextureCHROMIUM for all > > format/type combinations when doing texSubImage2D. > SRGB8 is not color-renderable, but SRGB8_ALPHA8 is, so why SRGB8_ALPHA8 needs > to be on a different path than, say, RGBA8? I put SRGB related formats here because current GPU-GPU copy for copying RGBA texture to SRGB texture does linear-to-srgb color space conversion. But, tThis conversion isn't done in current CPU uploading path in Blink. Current CTS tests doesn't expect the color space conversion either. So, I did a fallback path for thest SRGB formats. Are we talking about this?
On 2017/03/22 02:52:30, qiankun wrote: > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:16569: // resolved. > On 2017/03/21 08:47:13, qiankun wrote: > > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > > Can we not put them on DRAW_AND_READBACK path for now? So at least the > > > performance will be good. > > > > > We cannot detect internalformat in texSubImage2D in Blink. So, it's > impossible > > > to filter SRGB subImage uploading unless we disable CopyTextureCHROMIUM for > all > > > format/type combinations when doing texSubImage2D. > > > SRGB8 is not color-renderable, but SRGB8_ALPHA8 is, so why SRGB8_ALPHA8 needs > > to be on a different path than, say, RGBA8? > I put SRGB related formats here because current GPU-GPU copy for copying RGBA > texture to SRGB texture does linear-to-srgb color space conversion. But, tThis > conversion isn't done in current CPU uploading path in Blink. Current CTS tests > doesn't expect the color space conversion either. So, I did a fallback path for > thest SRGB formats. Are we talking about this? OK, let's not block this CL on this. Now I understand why you did it this way. Let's move forward as is and revisit later.
Description was changed from ========== Add readback path for CopyTextureCHROMIUM There are some liminations or driver bugs which cause GPU-GPU copy unavailable in CopyTextureCHROMIUM. For such cases, we do a readback fallback, i.e. draw the source texture to an intermediate texture in RGBA format, read back pixels of the intermediate texture from GPU to CPU, then upload pixels to destination texture with TexImage api. With this approach, the caller of glCopyTextureCHROMIUM doesn't need to care about the liminations and bugs mentioned above. BUG=612542 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add readback path for CopyTextureCHROMIUM There are some liminations or driver bugs which cause GPU-GPU copy unavailable in CopyTextureCHROMIUM. For such cases, we do a readback fallback, i.e. draw the source texture to an intermediate texture in RGBA format, read back pixels of the intermediate texture to a pbo, then upload pixels to destination texture with TexImage api. With this approach, the caller of glCopyTextureCHROMIUM doesn't need to care about the liminations and bugs mentioned above. BUG=612542 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:667: glReadPixels(0, 0, width, height, GL_RGBA, GL_UNSIGNED_BYTE, buf.get()); On 2017/03/21 08:47:13, qiankun wrote: > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > It would be really beneficial to explore the path of reading back to a > > PIXEL_PACK_BUFFER. Also, on desktop GL, you can directly read to a > > RGB/UNSIGNED_BYTE buffer or FLOAT buffer. On ES, you can probably use > > MapBufferRange to directly change PIXEL_PACK_BUFFER from RGBA to RGB; for > > floats, you probably can create a PIXEL_UNPACK_BUFFER and map it, and directly > > write to it. > This is a good suggestion. Thanks! I did experiment. This method works. > > > > > This could be a significant perf gain for these formats, but could you write > > some simple tests (repeatedly uploading textures to these formats) to measure > > the perf difference, so we have confidence that what we implement is optimal? > What kinds of test are in your mind, gl_tests or webgl tests? Should I add these > tests into this CL or just do some perf comparison locally? What the perf > difference should I measure, readback to client memory vs. readback to pbo? I did some experiments by running src/third_party/WebKit/PerformanceTests/Canvas/upload-canvas-2d-to-texture.html on some formats supported in WebGL 2.0. Here are the result on Linux with GeForce GTX 750, GL_VERSION=4.5.0 NVIDIA 364.19. The number means how many texImage2D calls can be done in 1 second. The larger, the better. Performance comparsion for readback path ------------------------------------------------------------------------ SRGB8_ALPHA8 RGB9_E5 Current_CPU_path_in_Blink: 46 20 Readback_to_client_memory_in_command_buffer: 275 18 Readback_to_pbo_in_command_buffer: 508 47 ------------------------------------------------------------------------ From the above table, we know readback path with pbo has the best performance. Performance comparison for readback path and GPU-GPU path ------------------------------------------------------------------------ Current_CPU_path_in_Blink GPU-GPU_path_in_command_buffer RGBA8UI: 49 4530 RGBA32F: 33 1632 RGB32F: 32 1553 RGB565: 52 4730 ------------------------------------------------------------------------ From the above table, we know GPU-GPU path is much faster than GPU-CPU-GPU path. About the benchmark, do you prefer we add tests for more formats under src/third_party/WebKit/PerformanceTests/Canvas/ or port the tests to Khronos/WebGL? https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16556: #if defined(OS_MACOSX) On 2017/03/21 08:47:13, qiankun wrote: > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > Can we hook this up with a driver bug workaround so we only limit it to NVidia > > on OSX? > > I can do this. I added the workaround for NVIDIA Mac. But it didn't detect correct driver vendor on a bot with two GPUs (Intel + NVIDIA). I will try it in a standalone CL after this CL is merged.. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:681: // pixel unpack buffer with glBufferData. I spent a lot of time on investigating this issue on Nexus 5. But I didn't find a better method to fix it. I used a workaround here. This code path only takes effect on RGB9_E5 formats. No problem when testing RGB9_E5, but the following tests for other formats after RGB9_E5 would fail randomly.
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.
Great work Qiankun! The perf data looks really awesome. A few minor issues, then we should be able to land this. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:681: // pixel unpack buffer with glBufferData. On 2017/03/29 09:00:10, qiankun wrote: > I spent a lot of time on investigating this issue on Nexus 5. But I didn't find > a better method to fix it. I used a workaround here. This code path only takes > effect on RGB9_E5 formats. No problem when testing RGB9_E5, but the following > tests for other formats after RGB9_E5 would fail randomly. This sounds like a memory corruption Qualcomm driver bug to me. Sorry this cost your time... https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:695: uint8_t* pixels = (uint8_t*)glMapBufferRange(GL_PIXEL_PACK_BUFFER, 0, This is against chromium coding style. Use cast. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:703: float* data = (float*)glMapBufferRange(GL_PIXEL_UNPACK_BUFFER, 0, buf_size, Same here, use cast. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:704: GL_MAP_WRITE_BIT); Combine with GL_MAP_INVALIDATE_BUFFER_BIT and see if it's faster. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:728: void DoReadbackAndTexImage(bool is_tex_image, It's better to define enum { kTexImage, kTexSubImage }. The code is much more readable. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:779: DCHECK(!is_es || decoder->GetFeatureInfo()->gl_version_info().is_es3); You can just DCHECK(!decoder->GetFeatureInfo()->gl_version_info().is_es2). https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:782: GLuint buffer[2]; Initialize buffer[0] and buffer[1] to 0.
On 2017/03/29 09:00:10, qiankun wrote: > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): > > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:667: > glReadPixels(0, 0, width, height, GL_RGBA, GL_UNSIGNED_BYTE, buf.get()); > On 2017/03/21 08:47:13, qiankun wrote: > > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > > It would be really beneficial to explore the path of reading back to a > > > PIXEL_PACK_BUFFER. Also, on desktop GL, you can directly read to a > > > RGB/UNSIGNED_BYTE buffer or FLOAT buffer. On ES, you can probably use > > > MapBufferRange to directly change PIXEL_PACK_BUFFER from RGBA to RGB; for > > > floats, you probably can create a PIXEL_UNPACK_BUFFER and map it, and > directly > > > write to it. > > This is a good suggestion. Thanks! I did experiment. This method works. > > > > > > > > This could be a significant perf gain for these formats, but could you write > > > some simple tests (repeatedly uploading textures to these formats) to > measure > > > the perf difference, so we have confidence that what we implement is > optimal? > > What kinds of test are in your mind, gl_tests or webgl tests? Should I add > these > > tests into this CL or just do some perf comparison locally? What the perf > > difference should I measure, readback to client memory vs. readback to pbo? > > I did some experiments by running > src/third_party/WebKit/PerformanceTests/Canvas/upload-canvas-2d-to-texture.html > on some formats supported in WebGL 2.0. Here are the result on Linux with > GeForce GTX 750, GL_VERSION=4.5.0 NVIDIA 364.19. The number means how many > texImage2D calls can be done in 1 second. The larger, the better. > Performance comparsion for readback path > ------------------------------------------------------------------------ > SRGB8_ALPHA8 RGB9_E5 > Current_CPU_path_in_Blink: 46 20 > Readback_to_client_memory_in_command_buffer: 275 18 > Readback_to_pbo_in_command_buffer: 508 47 > ------------------------------------------------------------------------ > From the above table, we know readback path with pbo has the best performance. > > > Performance comparison for readback path and GPU-GPU path > ------------------------------------------------------------------------ > Current_CPU_path_in_Blink GPU-GPU_path_in_command_buffer > RGBA8UI: 49 4530 > RGBA32F: 33 1632 > RGB32F: 32 1553 > RGB565: 52 4730 > ------------------------------------------------------------------------ > From the above table, we know GPU-GPU path is much faster than GPU-CPU-GPU path. > > > About the benchmark, do you prefer we add tests for more formats under > src/third_party/WebKit/PerformanceTests/Canvas/ or port the tests to > Khronos/WebGL? Yes, let's copy it to khronos github, under src/sdk/performance/textures, so other browsers can also test it. Yes, let's expand with more formats on github. > > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2760843002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:16556: #if defined(OS_MACOSX) > On 2017/03/21 08:47:13, qiankun wrote: > > On 2017/03/20 17:05:40, Zhenyao Mo wrote: > > > Can we hook this up with a driver bug workaround so we only limit it to > NVidia > > > on OSX? > > > > I can do this. > I added the workaround for NVIDIA Mac. But it didn't detect correct driver > vendor on a bot with two GPUs (Intel + NVIDIA). I will try it in a standalone CL > after this CL is merged.. Sounds good. > > https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): > > https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:681: // pixel > unpack buffer with glBufferData. > I spent a lot of time on investigating this issue on Nexus 5. But I didn't find > a better method to fix it. I used a workaround here. This code path only takes > effect on RGB9_E5 formats. No problem when testing RGB9_E5, but the following > tests for other formats after RGB9_E5 would fail randomly.
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.
Please review again. Thanks. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:681: // pixel unpack buffer with glBufferData. On 2017/03/29 17:29:34, Zhenyao Mo wrote: > On 2017/03/29 09:00:10, qiankun wrote: > > I spent a lot of time on investigating this issue on Nexus 5. But I didn't > find > > a better method to fix it. I used a workaround here. This code path only takes > > effect on RGB9_E5 formats. No problem when testing RGB9_E5, but the following > > tests for other formats after RGB9_E5 would fail randomly. > > This sounds like a memory corruption Qualcomm driver bug to me. Sorry this cost > your time... I see. Thanks for pointing it out to me. Per my testing, this workaround gains simialr performance as below implementation for RGB9_E5 format who will use this path. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:695: uint8_t* pixels = (uint8_t*)glMapBufferRange(GL_PIXEL_PACK_BUFFER, 0, On 2017/03/29 17:29:34, Zhenyao Mo wrote: > This is against chromium coding style. Use cast. Done. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:703: float* data = (float*)glMapBufferRange(GL_PIXEL_UNPACK_BUFFER, 0, buf_size, On 2017/03/29 17:29:34, Zhenyao Mo wrote: > Same here, use cast. Done. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:704: GL_MAP_WRITE_BIT); On 2017/03/29 17:29:34, Zhenyao Mo wrote: > Combine with GL_MAP_INVALIDATE_BUFFER_BIT and see if it's faster. The GL_MAP_INVALIDATE_BUFFER_BIT didn't have performance benefit per my testing. But, it will cause poor performance when I click the refresh button for a second round testing (texImage2D runs/sec: from 47 to 37 for RGB9_E5). So, I didn't use it here. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:728: void DoReadbackAndTexImage(bool is_tex_image, On 2017/03/29 17:29:34, Zhenyao Mo wrote: > It's better to define enum { kTexImage, kTexSubImage }. The code is much more > readable. Done. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:779: DCHECK(!is_es || decoder->GetFeatureInfo()->gl_version_info().is_es3); On 2017/03/29 17:29:34, Zhenyao Mo wrote: > You can just DCHECK(!decoder->GetFeatureInfo()->gl_version_info().is_es2). Done. https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:782: GLuint buffer[2]; On 2017/03/29 17:29:34, Zhenyao Mo wrote: > Initialize buffer[0] and buffer[1] to 0. Done.
lgtm https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/2760843002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:704: GL_MAP_WRITE_BIT); On 2017/03/30 07:56:07, qiankun wrote: > On 2017/03/29 17:29:34, Zhenyao Mo wrote: > > Combine with GL_MAP_INVALIDATE_BUFFER_BIT and see if it's faster. > > The GL_MAP_INVALIDATE_BUFFER_BIT didn't have performance benefit per my testing. > But, it will cause poor performance when I click the refresh button for a second > round testing (texImage2D runs/sec: from 47 to 37 for RGB9_E5). So, I didn't use > it here. That's quite the opposite from what we would expect with the INVALIDATE_BUFFER_BIT... very unfortunate
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": 80001, "attempt_start_ts": 1490925343174660,
"parent_rev": "7caded54fe6a6ab2db2fa9c8960699bae7fe1e5c", "commit_rev":
"986a2209658887e85eeb3e00c933db92383bef73"}
Message was sent while issue was closed.
Description was changed from ========== Add readback path for CopyTextureCHROMIUM There are some liminations or driver bugs which cause GPU-GPU copy unavailable in CopyTextureCHROMIUM. For such cases, we do a readback fallback, i.e. draw the source texture to an intermediate texture in RGBA format, read back pixels of the intermediate texture to a pbo, then upload pixels to destination texture with TexImage api. With this approach, the caller of glCopyTextureCHROMIUM doesn't need to care about the liminations and bugs mentioned above. BUG=612542 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add readback path for CopyTextureCHROMIUM There are some liminations or driver bugs which cause GPU-GPU copy unavailable in CopyTextureCHROMIUM. For such cases, we do a readback fallback, i.e. draw the source texture to an intermediate texture in RGBA format, read back pixels of the intermediate texture to a pbo, then upload pixels to destination texture with TexImage api. With this approach, the caller of glCopyTextureCHROMIUM doesn't need to care about the liminations and bugs mentioned above. BUG=612542 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2760843002 Cr-Commit-Position: refs/heads/master@{#460994} Committed: https://chromium.googlesource.com/chromium/src/+/986a2209658887e85eeb3e00c933... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/986a2209658887e85eeb3e00c933... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
