Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2273473003:
Reset GL state before copying RenderPassDrawQuads to CALayers. (Closed)
DescriptionReset GL state before copying RenderPassDrawQuads to CALayers.
Previously, GL state set by render pass drawing would be carried over.
BUG=640151
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/9800a6a28d9ed9fce1932bb0fc0c16fab5dc61de
Cr-Commit-Position: refs/heads/master@{#414144}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 17 (11 generated)
Description was changed from ========== Reset GL state before copying RenderPassDrawQuads to CALayers. Previously, GL state set by render pass drawing would be carried over. BUG=640151 ========== to ========== Reset GL state before copying RenderPassDrawQuads to CALayers. Previously, GL state set by render pass drawing would be carried over. BUG=640151 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
erikchen@chromium.org changed reviewers: + ericrk@chromium.org
ericrk: Please review.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by erikchen@chromium.org 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...
lgtm https://codereview.chromium.org/2273473003/diff/1/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/2273473003/diff/1/cc/output/gl_renderer.cc#ne... cc/output/gl_renderer.cc:3868: ReinitializeGLState(); This makes sense - so the state was just left over from previous rendering? Also, I wonder if we should do this higher up the chain (in ScheduleCALayers?) to avoid re-setting this state unnecessarily a bunch of times?
https://codereview.chromium.org/2273473003/diff/1/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/2273473003/diff/1/cc/output/gl_renderer.cc#ne... cc/output/gl_renderer.cc:3868: ReinitializeGLState(); On 2016/08/24 19:04:50, ericrk wrote: > This makes sense - so the state was just left over from previous rendering? Yup. Unfortunately, GLRenderer is very stateful. > > Also, I wonder if we should do this higher up the chain (in ScheduleCALayers?) > to avoid re-setting this state unnecessarily a bunch of times? I considered that. Most of the time, this function never gets called, so that would be unnecessary. We could add yet more logic to ScheduleCALayers to only call this once, and only right before CopyRenderPassDrawQuadToOverlayResource(). Then again, we're assuming that none of the drawing operations from this function modify state that needs to be reset between successive calls to this function. [This is currently true, but there's no reason it has to be true going forward.] Overall, reinitializing state here seemed like a good choice, especially given that the overhead is minimal compared to the cost of actual drawing operations.
The CQ bit was unchecked by erikchen@chromium.org
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Reset GL state before copying RenderPassDrawQuads to CALayers. Previously, GL state set by render pass drawing would be carried over. BUG=640151 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Reset GL state before copying RenderPassDrawQuads to CALayers. Previously, GL state set by render pass drawing would be carried over. BUG=640151 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/9800a6a28d9ed9fce1932bb0fc0c16fab5dc61de Cr-Commit-Position: refs/heads/master@{#414144} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/9800a6a28d9ed9fce1932bb0fc0c16fab5dc61de Cr-Commit-Position: refs/heads/master@{#414144} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
