Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496813002:
[Command buffer] workaround the framebuffer completeness bug for Intel Mac (Closed)
Descriptionworkaround the framebuffer completeness bug for Intel Mac.
If the read buffer or draw buffers have missing image,
but the fbo have image attached to other color buffer(s),
then reset corresponding readbuffer/drawbufers to GL_NONE,
before check framebuffer status for Intel Mac.
BUG=630800
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
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : apply the workaround to all Mac vendors, not only Mac Intel #
Total comments: 5
Patch Set 4 : confine to intel only #Patch Set 5 : don't need to check color buffers #Patch Set 6 : Another workaround: reset attachment point to GL_NONE if it has no image #Patch Set 7 : Reset (not restore) read buffer and/or draw buffers for all read/draw related APIs #
Total comments: 20
Patch Set 8 : addressed zmo@'s feedback, with some fixes to clear and clearbuffer* #Patch Set 9 : code refactoring per zmo@'s suggestion: move read/draw buffer adjustment into CheckBound{Read|Draw}FramebufferValid #
Total comments: 9
Patch Set 10 : code rebase #
Messages
Total messages: 60 (49 generated)
Description was changed from ========== coding style add the workaround for mac a small fix tried two method to fix the bug BUG= ========== to ========== coding style add the workaround for mac a small fix tried two method to fix the bug BUG= 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 ==========
The CQ bit was checked by yunchao.he@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...
Description was changed from ========== coding style add the workaround for mac a small fix tried two method to fix the bug BUG= 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 ========== workaround the framebuffer completeness bug for Intel Mac. If the read buffer or draw buffers have no image, but the fbo have image attached to other color buffer(s), then return FRAMEBUFFER_COMPLETE when check framebuffer status for Intel Mac. BUG=630800 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 ==========
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yunchao.he@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 checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by yunchao.he@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...
yunchao.he@intel.com changed reviewers: + kbr@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
PTAL. Thanks a lot!
The commit description should also be changed if this CL isn't Intel specific. https://codereview.chromium.org/2496813002/diff/140001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/2496813002/diff/140001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:144: # ['mac', ('nvidia', 0xfe9)], bug=616562) Can we remove this? https://codereview.chromium.org/2496813002/diff/140001/gpu/command_buffer/ser... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2496813002/diff/140001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:640: if (attachments_.find(GL_COLOR_ATTACHMENT0 + i) != attachments_.end()) Why don't check attachment != NULL (i.e. attachments_.find(GL_COLOR_ATTACHMENT0 + i)->second) here? What's the difference of Drawbuffers and ColorBuffers? Aren't they same? https://codereview.chromium.org/2496813002/diff/140001/gpu/config/gpu_driver_... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/2496813002/diff/140001/gpu/config/gpu_driver_... gpu/config/gpu_driver_bug_list_json.cc:2214: "description": "return framebuffer complete if read buffer or draw buffers have no image on Intel Mac", return -> Return This workaround should be not Intel specific.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
See my comments on crbug.com/630800 I don't think this is a good workaround.
The CQ bit was checked by yunchao.he@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 yunchao.he@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 checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/11 21:21:49, Zhenyao Mo wrote: > See my comments on crbug.com/630800 > > I don't think this is a good workaround. Hi, Zhenyao and all, The latest patch set is basically finished, you can take a look if you have time. The general idea is: Set readBuffer/drawBuffers to GL_NONE if they have no image attached for checkFramebufferStatus. Unfortunately, after resetting (We changed COLOR_ATTACHMENTi to GL_NONE during resetting), If there is no valid READ_BUFFER / DRAW_BUFFERS(all of them are GL_NONE), the driver still think it is framebuffer incomplete. So return FRAMEBUFFER_COMPLETE directly for this situation. For all read/draw related APIs, I don't use Restore(), but use the same function to Reset read buffer and/or draw buffers. Because some read buffer and/or draw buffers may have no image too (even framebuffer state changed, we have a flag for this). So, To restore to its previous status is not correct. If would fail because of framebuffer incomplete. So I use reset too, as what I do for checkFramebufferStatus. After reset, If READ_BUFFER/DRAW_BUFFERS have no valid attachments(all of them are GL_NONE after resetting), the underlying driver still think it is framebuffer incomplete, so I need to handle them before they go to driver: return INVALID_OPERATION for reading, and return NO_ERROR for drawing and draw nothing. Some work still need to do: 1) The similar code for ClearBuffer in GLES2DecoderImpl. 2) don't use constant 8 for buf[] in GLES2DecoderImpl::AdjustColorBufferAttachmentsIfNecessary() (But It is too late at my time zone, I will do this tomorrow).
Yunchao, thanks for working on this. I had some high level design suggestions. I reviewed part of the CL details, but it's not complete. I'll do a complete review once it's revised. https://codereview.chromium.org/2496813002/diff/220001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2496813002/diff/220001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:137: ['mac', ('nvidia', 0xfe9)], bug=616562) Move this to Mac NVidia section https://codereview.chromium.org/2496813002/diff/220001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:294: # ['mac', 'amd'], bug=483282) Let's add a workaround entry for Mac AMD where OSX < 10.12.2, so we can remove this entry also. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:619: if (it == attachments_.end()) { Just return it == attachments_.end(). https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:634: buf[i] = GL_COLOR_ATTACHMENT0 + i; Here you should DCHECK(buf[i] == GL_COLOR_ATTACHMENT0 + i). No need to re-assign the value. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:635: ++num; Here you assume num is initialized to zero by caller, which is bug prone. ANyway, I don't think you need to care about num at all. Just get rid of it. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/framebuffer_manager.h (right): https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.h:151: bool DrawBuffersHaveNoImage(GLsizei& num, GLenum* buf) const; 1) I think DrawBuffersHaveMissingImage is a better name. Otherwise it's confusing because it could be read as "no images are attached to any draw buffers". 2) "&num": this is against chromium coding style. Use GLsizei* num instead. 3) Why do we need num? I think we can just get rid of it. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:1435: // if the specified read buffer or draw buffers have no image attached. have missing images. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7224: if (mask & GL_COLOR_BUFFER_BIT && (mask & GL_COLOR_BUFFER_BIT) != 0 https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9892: if (!CheckBoundDrawFramebufferValid(function_name)) { All this same code is right above CheckBoundDrawFramebufferValid or CheckBoundFramebufferValid. It looks to me that the workaround should just be part of that function, so we don't have to repeat ourselves all over the places. https://codereview.chromium.org/2496813002/diff/220001/gpu/config/gpu_driver_... File gpu/config/gpu_driver_bug_workaround_type.h (right): https://codereview.chromium.org/2496813002/diff/220001/gpu/config/gpu_driver_... gpu/config/gpu_driver_bug_workaround_type.h:19: adjust_framebuffer_complete_status) \ I prefer a name like missing_read_draw_buffer_workaround
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #10 (id:280001) has been deleted
Patchset #8 (id:240001) has been deleted
The CQ bit was checked by yunchao.he@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...
Thanks for your review, Qiankun. https://codereview.chromium.org/2496813002/diff/140001/gpu/command_buffer/ser... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2496813002/diff/140001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:640: if (attachments_.find(GL_COLOR_ATTACHMENT0 + i) != attachments_.end()) On 2016/11/11 15:30:48, qiankun wrote: > Why don't check attachment != NULL (i.e. attachments_.find(GL_COLOR_ATTACHMENT0 > + i)->second) here? It is unnecessary. see the other code like HasColorAttachment, etc. The AttachmentMap will erase or attach the first and the second all together. > > What's the difference of Drawbuffers and ColorBuffers? Aren't they same? Maybe the name is a little confusing, but draw buffers are per context, color buffers are per fbo. https://codereview.chromium.org/2496813002/diff/140001/gpu/config/gpu_driver_... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/2496813002/diff/140001/gpu/config/gpu_driver_... gpu/config/gpu_driver_bug_list_json.cc:2214: "description": "return framebuffer complete if read buffer or draw buffers have no image on Intel Mac", On 2016/11/11 15:30:48, qiankun wrote: > return -> Return > This workaround should be not Intel specific. Done.
Thanks for your review, Zhenyao. Could you take another look at the latest patch set? https://codereview.chromium.org/2496813002/diff/220001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2496813002/diff/220001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:137: ['mac', ('nvidia', 0xfe9)], bug=616562) On 2016/11/14 19:42:24, Zhenyao Mo wrote: > Move this to Mac NVidia section Done. https://codereview.chromium.org/2496813002/diff/220001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:294: # ['mac', 'amd'], bug=483282) On 2016/11/14 19:42:24, Zhenyao Mo wrote: > Let's add a workaround entry for Mac AMD where OSX < 10.12.2, so we can remove > this entry also. It can pass on Mac AMD without any extra code. I have removed this. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:619: if (it == attachments_.end()) { On 2016/11/14 19:42:24, Zhenyao Mo wrote: > Just return it == attachments_.end(). Done. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:634: buf[i] = GL_COLOR_ATTACHMENT0 + i; On 2016/11/14 19:42:24, Zhenyao Mo wrote: > Here you should DCHECK(buf[i] == GL_COLOR_ATTACHMENT0 + i). No need to re-assign > the value. Not re-assign. The buf[] passed in are all GL_NONE. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:635: ++num; On 2016/11/14 19:42:24, Zhenyao Mo wrote: > Here you assume num is initialized to zero by caller, which is bug prone. > > ANyway, I don't think you need to care about num at all. Just get rid of it. See the explanation elsewhere. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/framebuffer_manager.h (right): https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.h:151: bool DrawBuffersHaveNoImage(GLsizei& num, GLenum* buf) const; On 2016/11/14 19:42:24, Zhenyao Mo wrote: > 1) I think DrawBuffersHaveMissingImage is a better name. Otherwise it's > confusing because it could be read as "no images are attached to any draw > buffers". Agreed. > > 2) "&num": this is against chromium coding style. Use GLsizei* num instead. Done. > > 3) Why do we need num? I think we can just get rid of it. Mac Intel driver still think it is framebuffer incomplete if 1) we reset COLOR_ATTACHMENTi to GL_NONE for readBuffer or drawBuffers those are missing image, 2)After reset, there is not valid read/draw buffer(all of them are GL_NONE after reset). As a result, use parameter num to record the valid drawbuffers number after reset. Generate INVALID_OPERATION （read） or return directly (draw) in Chromium if num is 0. Don't go to driver to check framebuffer status for this situation. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:1435: // if the specified read buffer or draw buffers have no image attached. On 2016/11/14 19:42:24, Zhenyao Mo wrote: > have missing images. Done. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7224: if (mask & GL_COLOR_BUFFER_BIT && On 2016/11/14 19:42:24, Zhenyao Mo wrote: > (mask & GL_COLOR_BUFFER_BIT) != 0 Done. https://codereview.chromium.org/2496813002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9892: if (!CheckBoundDrawFramebufferValid(function_name)) { On 2016/11/14 19:42:24, Zhenyao Mo wrote: > All this same code is right above CheckBoundDrawFramebufferValid or > CheckBoundFramebufferValid. It looks to me that the workaround should just be > part of that function, so we don't have to repeat ourselves all over the places. Done. It is a little different in Clear and ClearBuffer*, so, still handle it like this in the callers for these functions. https://codereview.chromium.org/2496813002/diff/220001/gpu/config/gpu_driver_... File gpu/config/gpu_driver_bug_workaround_type.h (right): https://codereview.chromium.org/2496813002/diff/220001/gpu/config/gpu_driver_... gpu/config/gpu_driver_bug_workaround_type.h:19: adjust_framebuffer_complete_status) \ On 2016/11/14 19:42:24, Zhenyao Mo wrote: > I prefer a name like missing_read_draw_buffer_workaround That name is not good. What about read_draw_buffer_missing_image？
> https://codereview.chromium.org/2496813002/diff/220001/gpu/config/gpu_driver_... > gpu/config/gpu_driver_bug_workaround_type.h:19: > adjust_framebuffer_complete_status) \ > On 2016/11/14 19:42:24, Zhenyao Mo wrote: > > I prefer a name like missing_read_draw_buffer_workaround > > That name is not good. What about read_draw_buffer_missing_image？ I mean my previous name for the workaround "adjust_framebuffer_complete_status" is not good... :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== workaround the framebuffer completeness bug for Intel Mac. If the read buffer or draw buffers have no image, but the fbo have image attached to other color buffer(s), then return FRAMEBUFFER_COMPLETE when check framebuffer status for Intel Mac. BUG=630800 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 ========== workaround the framebuffer completeness bug for Intel Mac. If the read buffer or draw buffers have missing image, but the fbo have image attached to other color buffer(s), then reset corresponding readbuffer/drawbufers to GL_NONE, before check framebuffer status for Intel Mac. BUG=630800 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 ==========
This CL still needs some careful thinking about designing. At this point, I would suggest we move it to after M56 branching point because this is a high risk CL. In general, I think this CL can be way simpler than it is right now. You just need to find a common place to apply this workaround. It should also only apply when the cached state is dirty. Otherwise it will cause a perf regression (I have no idea the impact, but we should definitely avoid it since we can). So the viable place to put it is Framebuffer::GetStatus, where the state caching is checked, etc. https://codereview.chromium.org/2496813002/diff/300001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2496813002/diff/300001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:135: ['mac'], bug=630800) What about this one? They are the same root cause. https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:644: for (GLenum i = 0; i < manager_->max_color_attachments_; i++) { nit: ++i https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/framebuffer_manager.cc:644: for (GLenum i = 0; i < manager_->max_color_attachments_; i++) { Can you do the other way? i.e., go through attachments_ (in most cases it has only a few), and see if their key is between GL_COLOR_ATTACHMENT0 and GL_COLOR_ATTACHMENT0 + max_color_attchments https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:1303: bool may_need_workaround); may_need_workaround should always be true. i.e., we can just remove it. https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:1308: bool may_need_workaround); Same here, see my comments below. https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:1448: bool AdjustColorBufferAttachmentsIfNecessary( It should really be AdjustReadBufferAndDrawBuffersIfNecessary(). Also, to be simple, you could always adjust read and draw, so no need for the following two args. https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7253: if ((mask & GL_COLOR_BUFFER_BIT) != 0 && This is wrong. See below. https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7296: if (SpecifiedDrawBufferHasNoImage(drawbuffer)) This is very wrong. It's not just the image that you try to clear that's missing will cause CheckFramebufferStatus to return wrong value, it's any not NONE draw buffer with missing image will trigger this problem. https://codereview.chromium.org/2496813002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7603: bool GLES2DecoderImpl::SpecifiedDrawBufferHasNoImage(GLint ii) { No need for this function.
On 2016/11/16 03:27:28, Zhenyao Mo wrote: > This CL still needs some careful thinking about designing. > > At this point, I would suggest we move it to after M56 branching point because > this is a high risk CL. Make sense. readbuffer and drawbuffers missing image is not well covered by WebGL CTS. So the bots can not catch errors. I use https://github.com/KhronosGroup/WebGL/pull/2137 to validate this patch. But that pr is not landed too.
On 2016/11/16 06:04:54, yunchao wrote: > On 2016/11/16 03:27:28, Zhenyao Mo wrote: > > This CL still needs some careful thinking about designing. > > > > At this point, I would suggest we move it to after M56 branching point because > > this is a high risk CL. > > Make sense. readbuffer and drawbuffers missing image is not well covered by > WebGL CTS. So the bots can not catch errors. > > I use https://github.com/KhronosGroup/WebGL/pull/2137 to validate this patch. > But that pr is not landed too. As Zhenyao said at the corresponding crbug/630800. This workaround seems to be nasty. And we have already filed radar (29715965) to Apple. I would like to close this patch. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
