Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(519)

Issue 2268503002: [Command Buffer] enable/disable FRAMEBUFFER_SRGB only when sRGB image is active (Closed)

Created:
4 years, 4 months ago by yunchao
Modified:
4 years, 2 months ago
CC:
chromium-reviews, piman+watch_chromium.org, Yang Gu, yizhou.jiang
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Command Buffer] enable/disable FRAMEBUFFER_SRGB only when sRGB image is active. If there are sRGB images in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. This small change can reduce the frequency of check and switch framebuffer srgb state. 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

Patch Set 1 #

Patch Set 2 : remove unnecessary code #

Patch Set 3 : encapusulate some code to a function #

Patch Set 4 : framebuffer_srgb_ is not context-aware, set it to false during context restore #

Patch Set 5 : framebuffer_srgb_ is not context-aware, set it to true during context restore #

Patch Set 6 : encapusulate some code to a function #

Total comments: 10

Patch Set 7 : addressed zmo's feedback, small optimization and fix typos #

Total comments: 1

Patch Set 8 : typo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -32 lines) Patch
M gpu/command_buffer/service/gles2_cmd_copy_tex_image.cc View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M gpu/command_buffer/service/gles2_cmd_decoder.cc View 1 2 3 4 5 6 7 12 chunks +62 lines, -30 lines 0 comments Download

Messages

Total messages: 68 (59 generated)
yunchao
Some comments from https://codereview.chromium.org/2246823002/: I am looking at the sRGB related issue(emulate sRGB for core ...
4 years, 4 months ago (2016-08-24 14:02:04 UTC) #50
yunchao
See the comments inline. https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/service/context_state.cc File gpu/command_buffer/service/context_state.cc (left): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/service/context_state.cc#oldcode503 gpu/command_buffer/service/context_state.cc:503: } else if (framebuffer_srgb_ != ...
4 years, 4 months ago (2016-08-24 14:08:51 UTC) #51
qiankun
https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/service/context_state.cc File gpu/command_buffer/service/context_state.cc (left): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/service/context_state.cc#oldcode503 gpu/command_buffer/service/context_state.cc:503: } else if (framebuffer_srgb_ != prev_state->framebuffer_srgb_) { On 2016/08/24 ...
4 years, 4 months ago (2016-08-24 16:21:01 UTC) #52
qiankun
https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/service/context_state.cc File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/service/context_state.cc#newcode499 gpu/command_buffer/service/context_state.cc:499: // FRAMEBUFFER_SRGB will be restored lazily at render time. ...
4 years, 4 months ago (2016-08-24 16:22:14 UTC) #53
Zhenyao Mo
https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/service/context_state.cc File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/service/context_state.cc#newcode503 gpu/command_buffer/service/context_state.cc:503: EnableDisable(GL_FRAMEBUFFER_SRGB, framebuffer_srgb_); That's what we try to avoid. If ...
4 years, 3 months ago (2016-08-25 00:39:38 UTC) #54
yunchao
Thanks for your review, Zhenyao. You are correct, all bots can run correctly when framebuffer_srgb ...
4 years, 3 months ago (2016-08-25 16:29:51 UTC) #59
Ken Russell (switch to Gerrit)
I'm not an owner of this code, but I'm sorry, I don't think this is ...
4 years, 3 months ago (2016-08-25 18:22:46 UTC) #64
Zhenyao Mo
On 2016/08/25 18:22:46, Ken Russell wrote: > I'm not an owner of this code, but ...
4 years, 3 months ago (2016-08-25 18:25:55 UTC) #65
yunchao
4 years, 3 months ago (2016-08-26 01:56:15 UTC) #68
On 2016/08/25 18:25:55, Zhenyao Mo wrote:
> On 2016/08/25 18:22:46, Ken Russell wrote:
> > I'm not an owner of this code, but I'm sorry, I don't think this is a good
> > direction for the code. EnableDisableFramebufferSRGBForDrawBuffers is being
> > called just before draw calls like DrawElements, and it iterates down all of
> the
> > bound draw buffers, which is likely an expensive check. I think the way this
> is
> > done in the current code is better.
> 
> I agree with Ken.
> 
> Also, you missed a bunch of calls, like instanced draw calls.  That's why we
put
> it in the frambuffer completeness check - to avoid making mistakes like this.

To Ken, the draw buffers are cached in frame_buffer_manager, so I suppose this
is not expensive. 
To Zhenyao, Instanced drawing are wrapped into corresponding
DrawElements/DrawArrays, so no drawing APIs are missed. 
However, I indeed don't check/switch the framebuffer_srgb for nv_path_rendering
APIs, like glStencilFillPathNV and so forth. 

Well, this change is not a must. I will close it.

Powered by Google App Engine
This is Rietveld 408576698