Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2420813002:
optimize precision for conversion from srgb to linear (Closed)
|
Created:
4 years, 2 months ago by yizhou.jiang Modified:
4 years, 1 month ago CC:
chromium-reviews, piman+watch_chromium.org, Yang Gu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionoptimize precision for conversion from srgb to linear
Convertion from sRGB to 8-bit linear is going to reduce the
precision of the texture, and the downsampling will do the
same thing. We should set texture internalformat to rgba32f
to optimize precision.
BUG=634519
TEST=conformance2/textures/misc/tex-srgb-mipmap.html
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/0500571c0a41d977f64653eeaed4809351205a35
Cr-Commit-Position: refs/heads/master@{#427014}
Patch Set 1 : optimize precision for conversion from srgb to linear #
Total comments: 2
Patch Set 2 : Addressed to Qiankun's feedback #Patch Set 3 : Addressed to Zhenyao's feedback #
Total comments: 2
Patch Set 4 : Addressed to Zhenyao's feedback #Patch Set 5 : rebase code #
Messages
Total messages: 60 (38 generated)
Description was changed from ========== set texture internalformat to rgba32f to optimize precision BUG=634519 ========== to ========== set texture internalformat to rgba32f to optimize precision BUG=634519 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 ==========
Description was changed from ========== set texture internalformat to rgba32f to optimize precision BUG=634519 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 ========== Conversation from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ==========
yizhou.jiang@intel.com changed reviewers: + yunchao.he@intel.com
The CQ bit was checked by yizhou.jiang@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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by yizhou.jiang@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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
yunchao.he@intel.com changed reviewers: + brianosman@google.com, kbr@chromium.org, zmo@chromium.org
LGTM. @zmo and all, PTAL.
The CQ bit was checked by yizhou.jiang@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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by yizhou.jiang@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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by yizhou.jiang@intel.com
On 2016/10/14 06:29:52, yunchao wrote: > LGTM. > @zmo and all, PTAL. Zhenyao and Ken, could you grant Yizhou(yizhou.jiang@intel.com) the privilege to run bots? When she try to run bots in trybots, or click CQ dry run, the bots will fail to run. I suppose that she is not granted the privilege. Thanks in advance.
yunchao.he@intel.com changed reviewers: + qiankun.miao@intel.com
On 2016/10/14 06:29:52, yunchao wrote: > LGTM. > @zmo and all, PTAL. Thanks for your review, yunchao. @zmo @Brianosman @kbr PTAL. Btw, the 'choose trybots' always reports 'an error occrred while trying to update this issue'.
Description was changed from ========== Conversation from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ========== Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ==========
https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:361: glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA32F, width, height, 0, GL_RGBA, RGBA32F isn't color renderable or texture filterable by default, so a bunch of calls, including generateMipmap will fail. You have to do either extension check (I don't know the desktop situation - you should read the spec there), or use the color_renderable and texture_filterable validators to make sure.
Description was changed from ========== Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ==========
Description was changed from ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ==========
Description was changed from ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ==========
Description was changed from ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ==========
Description was changed from ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ==========
Thanks for your quick review, Zhenyao. Please see the comments inline. https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:361: glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA32F, width, height, 0, GL_RGBA, On 2016/10/14 07:05:04, Zhenyao Mo wrote: > RGBA32F isn't color renderable or texture filterable by default, so a bunch of > calls, including generateMipmap will fail. You have to do either extension > check (I don't know the desktop situation - you should read the spec there), or > use the color_renderable and texture_filterable validators to make sure. In the OGL spec, RGBA32F is color-renderable (although it is not color-renderable in GLES spec). But there is no words about texture-filterable, but it is an effective texture internalformat. I think it is texture filterable now that no words says it can not to be. We tested the conformance test with this patch both on Linux mesa 3.3 and Mac Intel (OGL 4.1), it can pass without any error. Now that it can be support by default. I suppose it is OK without extra check. What do you think?
On 2016/10/14 07:28:34, yunchao wrote: > Thanks for your quick review, Zhenyao. Please see the comments inline. > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:361: > glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA32F, width, height, 0, GL_RGBA, > On 2016/10/14 07:05:04, Zhenyao Mo wrote: > > RGBA32F isn't color renderable or texture filterable by default, so a bunch of > > calls, including generateMipmap will fail. You have to do either extension > > check (I don't know the desktop situation - you should read the spec there), > or > > use the color_renderable and texture_filterable validators to make sure. > > In the OGL spec, RGBA32F is color-renderable (although it is not > color-renderable in GLES spec). But there is no words about texture-filterable, > but it is an effective texture internalformat. I think it is texture filterable > now that no words says it can not to be. > > We tested the conformance test with this patch both on Linux mesa 3.3 and Mac > Intel (OGL 4.1), it can pass without any error. > > Now that it can be support by default. I suppose it is OK without extra check. > What do you think? This can still fail on ES3 without EXT_color_buffer_float extension. Basically you will want to find a way to check if FeatureInfo::ext_color_buffer_float_available_ is true here. If not, RGBA8 is your fallback.
On 2016/10/14 17:13:51, Zhenyao Mo wrote: > On 2016/10/14 07:28:34, yunchao wrote: > > Thanks for your quick review, Zhenyao. Please see the comments inline. > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:361: > > glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA32F, width, height, 0, GL_RGBA, > > On 2016/10/14 07:05:04, Zhenyao Mo wrote: > > > RGBA32F isn't color renderable or texture filterable by default, so a bunch > of > > > calls, including generateMipmap will fail. You have to do either extension > > > check (I don't know the desktop situation - you should read the spec there), > > or > > > use the color_renderable and texture_filterable validators to make sure. > > > > In the OGL spec, RGBA32F is color-renderable (although it is not > > color-renderable in GLES spec). But there is no words about > texture-filterable, > > but it is an effective texture internalformat. I think it is texture > filterable > > now that no words says it can not to be. > > > > We tested the conformance test with this patch both on Linux mesa 3.3 and Mac > > Intel (OGL 4.1), it can pass without any error. > > > > Now that it can be support by default. I suppose it is OK without extra check. > > What do you think? > > This can still fail on ES3 without EXT_color_buffer_float extension. > > Basically you will want to find a way to check if > FeatureInfo::ext_color_buffer_float_available_ is true here. > If not, RGBA8 is your fallback. Hi, Zhenyao, per my understanding, the code will be executed only in OpenGL profile. In OpenGL, RGBA32F can be support directly, just like RGBA8 can be support directly. However, it is not necessary to emulate srgb on ES3. The emulation is not necessary in ES3 profile. So I still think it is not necessary to check and fallback. Maybe I am wrong, please correct me if so.
On 2016/10/15 01:11:38, yunchao wrote: > On 2016/10/14 17:13:51, Zhenyao Mo wrote: > > On 2016/10/14 07:28:34, yunchao wrote: > > > Thanks for your quick review, Zhenyao. Please see the comments inline. > > > > > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > > > > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:361: > > > glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA32F, width, height, 0, GL_RGBA, > > > On 2016/10/14 07:05:04, Zhenyao Mo wrote: > > > > RGBA32F isn't color renderable or texture filterable by default, so a > bunch > > of > > > > calls, including generateMipmap will fail. You have to do either > extension > > > > check (I don't know the desktop situation - you should read the spec > there), > > > or > > > > use the color_renderable and texture_filterable validators to make sure. > > > > > > In the OGL spec, RGBA32F is color-renderable (although it is not > > > color-renderable in GLES spec). But there is no words about > > texture-filterable, > > > but it is an effective texture internalformat. I think it is texture > > filterable > > > now that no words says it can not to be. > > > > > > We tested the conformance test with this patch both on Linux mesa 3.3 and > Mac > > > Intel (OGL 4.1), it can pass without any error. > > > > > > Now that it can be support by default. I suppose it is OK without extra > check. > > > What do you think? > > > > This can still fail on ES3 without EXT_color_buffer_float extension. > > > > Basically you will want to find a way to check if > > FeatureInfo::ext_color_buffer_float_available_ is true here. > > If not, RGBA8 is your fallback. > > Hi, Zhenyao, per my understanding, the code will be executed only in OpenGL > profile. > In OpenGL, RGBA32F can be support directly, just like RGBA8 can be support > directly. > However, it is not necessary to emulate srgb on ES3. The emulation is not > necessary in ES3 profile. > So I still think it is not necessary to check and fallback. > > Maybe I am wrong, please correct me if so. This is for generateMipmap, right? I think even ES3 needs that workaround. Am I wrong?
On 2016/10/15 02:53:47, Zhenyao Mo wrote: > On 2016/10/15 01:11:38, yunchao wrote: > > On 2016/10/14 17:13:51, Zhenyao Mo wrote: > > > On 2016/10/14 07:28:34, yunchao wrote: > > > > Thanks for your quick review, Zhenyao. Please see the comments inline. > > > > > > > > > > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > > > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:361: > > > > glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA32F, width, height, 0, GL_RGBA, > > > > On 2016/10/14 07:05:04, Zhenyao Mo wrote: > > > > > RGBA32F isn't color renderable or texture filterable by default, so a > > bunch > > > of > > > > > calls, including generateMipmap will fail. You have to do either > > extension > > > > > check (I don't know the desktop situation - you should read the spec > > there), > > > > or > > > > > use the color_renderable and texture_filterable validators to make sure. > > > > > > > > In the OGL spec, RGBA32F is color-renderable (although it is not > > > > color-renderable in GLES spec). But there is no words about > > > texture-filterable, > > > > but it is an effective texture internalformat. I think it is texture > > > filterable > > > > now that no words says it can not to be. > > > > > > > > We tested the conformance test with this patch both on Linux mesa 3.3 and > > Mac > > > > Intel (OGL 4.1), it can pass without any error. > > > > > > > > Now that it can be support by default. I suppose it is OK without extra > > check. > > > > What do you think? > > > > > > This can still fail on ES3 without EXT_color_buffer_float extension. > > > > > > Basically you will want to find a way to check if > > > FeatureInfo::ext_color_buffer_float_available_ is true here. > > > If not, RGBA8 is your fallback. > > > > Hi, Zhenyao, per my understanding, the code will be executed only in OpenGL > > profile. > > In OpenGL, RGBA32F can be support directly, just like RGBA8 can be support > > directly. > > However, it is not necessary to emulate srgb on ES3. The emulation is not > > necessary in ES3 profile. > > So I still think it is not necessary to check and fallback. > > > > Maybe I am wrong, please correct me if so. > > This is for generateMipmap, right? I think even ES3 needs that workaround. Am I > wrong? Zhenyao, your feedback reminded me that we should use high precision texture format for srgb conversion for blitFramebuffer too. Could you also take a look at https://codereview.chromium.org/2419153003/. For blitFramebuffer, it is definitely only for OGL profile, not for ES3 profile. The ES3 spec explicitly says it can support srgb conversion during blitFramebuffer without any emulation. Per generateMipmap, it is a little complicated. Both the OGL spec and GLES spec don't say whether it can support srgb conversion during scaling. But the actual tests show that: if blitFramebuffer can convert srgb during blitting, generateMipmap can convert srgb too (on OGL 4.4 and above), otherwise, if blitFramebuffer can not handle srgb conversion, generateMipmap will not handle srgb conversion directly during scaling. It is reasonable for driver implementation: convert srgb or not for all related APIs. So I suppose that on GLES3, it can convert srgb directly for generateMipmap without emulation too. And this patch (https://codereview.chromium.org/2318313004/) only add workaround to emulate srgb for generateMipmap API for OGL profile, on which RGBA32F can be support directly. Maybe that behavior is not correct. Maybe srgb emulation for generateMipmap is necessary for GLES3 profile too. So, should we verify both this fix and https://codereview.chromium.org/2318313004/ on more device with ES3 profile?
On 2016/10/15 15:14:39, yunchao wrote: > On 2016/10/15 02:53:47, Zhenyao Mo wrote: > > On 2016/10/15 01:11:38, yunchao wrote: > > > On 2016/10/14 17:13:51, Zhenyao Mo wrote: > > > > On 2016/10/14 07:28:34, yunchao wrote: > > > > > Thanks for your quick review, Zhenyao. Please see the comments inline. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > > > > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > > > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:361: > > > > > glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA32F, width, height, 0, GL_RGBA, > > > > > On 2016/10/14 07:05:04, Zhenyao Mo wrote: > > > > > > RGBA32F isn't color renderable or texture filterable by default, so a > > > bunch > > > > of > > > > > > calls, including generateMipmap will fail. You have to do either > > > extension > > > > > > check (I don't know the desktop situation - you should read the spec > > > there), > > > > > or > > > > > > use the color_renderable and texture_filterable validators to make > sure. > > > > > > > > > > In the OGL spec, RGBA32F is color-renderable (although it is not > > > > > color-renderable in GLES spec). But there is no words about > > > > texture-filterable, > > > > > but it is an effective texture internalformat. I think it is texture > > > > filterable > > > > > now that no words says it can not to be. > > > > > > > > > > We tested the conformance test with this patch both on Linux mesa 3.3 > and > > > Mac > > > > > Intel (OGL 4.1), it can pass without any error. > > > > > > > > > > Now that it can be support by default. I suppose it is OK without extra > > > check. > > > > > What do you think? > > > > > > > > This can still fail on ES3 without EXT_color_buffer_float extension. > > > > > > > > Basically you will want to find a way to check if > > > > FeatureInfo::ext_color_buffer_float_available_ is true here. > > > > If not, RGBA8 is your fallback. > > > > > > Hi, Zhenyao, per my understanding, the code will be executed only in OpenGL > > > profile. > > > In OpenGL, RGBA32F can be support directly, just like RGBA8 can be support > > > directly. > > > However, it is not necessary to emulate srgb on ES3. The emulation is not > > > necessary in ES3 profile. > > > So I still think it is not necessary to check and fallback. > > > > > > Maybe I am wrong, please correct me if so. > > > > This is for generateMipmap, right? I think even ES3 needs that workaround. Am > I > > wrong? > > Zhenyao, your feedback reminded me that we should use high precision texture > format for srgb conversion for blitFramebuffer too. Could you also take a look > at https://codereview.chromium.org/2419153003/. For blitFramebuffer, it is > definitely only for OGL profile, not for ES3 profile. The ES3 spec explicitly > says it can support srgb conversion during blitFramebuffer without any > emulation. > > Per generateMipmap, it is a little complicated. Both the OGL spec and GLES spec > don't say whether it can support srgb conversion during scaling. But the actual > tests show that: if blitFramebuffer can convert srgb during blitting, > generateMipmap can convert srgb too (on OGL 4.4 and above), otherwise, if > blitFramebuffer can not handle srgb conversion, generateMipmap will not handle > srgb conversion directly during scaling. It is reasonable for driver > implementation: convert srgb or not for all related APIs. So I suppose that on > GLES3, it can convert srgb directly for generateMipmap without emulation too. > And this patch (https://codereview.chromium.org/2318313004/) only add workaround > to emulate srgb for generateMipmap API for OGL profile, on which RGBA32F can be > support directly. > > Maybe that behavior is not correct. Maybe srgb emulation for generateMipmap is > necessary for GLES3 profile too. > So, should we verify both this fix and > https://codereview.chromium.org/2318313004/ on more device with ES3 profile? I can test on one of the ES3 drivers and see if generateMipmap with sRGB works there. The reason we want to be more careful here is we don't want to leak a GL error in the future (who knows which platforms we may turn on this workaround in the future).
> > Zhenyao, your feedback reminded me that we should use high precision texture > > format for srgb conversion for blitFramebuffer too. Could you also take a look > > at https://codereview.chromium.org/2419153003/. For blitFramebuffer, it is > > definitely only for OGL profile, not for ES3 profile. The ES3 spec explicitly > > says it can support srgb conversion during blitFramebuffer without any > > emulation. > > > > Per generateMipmap, it is a little complicated. Both the OGL spec and GLES > spec > > don't say whether it can support srgb conversion during scaling. But the > actual > > tests show that: if blitFramebuffer can convert srgb during blitting, > > generateMipmap can convert srgb too (on OGL 4.4 and above), otherwise, if > > blitFramebuffer can not handle srgb conversion, generateMipmap will not handle > > srgb conversion directly during scaling. It is reasonable for driver > > implementation: convert srgb or not for all related APIs. So I suppose that on > > GLES3, it can convert srgb directly for generateMipmap without emulation too. > > And this patch (https://codereview.chromium.org/2318313004/) only add > workaround > > to emulate srgb for generateMipmap API for OGL profile, on which RGBA32F can > be > > support directly. > > > > Maybe that behavior is not correct. Maybe srgb emulation for generateMipmap is > > necessary for GLES3 profile too. > > So, should we verify both this fix and > > https://codereview.chromium.org/2318313004/ on more device with ES3 profile? > > I can test on one of the ES3 drivers and see if generateMipmap with sRGB works > there. > > The reason we want to be more careful here is we don't want to leak a GL error > in the future (who knows which platforms we may turn on this workaround in the > future). That's true. The GLES/OGL spec don't talk about this for generateMipmap. Maybe we need to do srgb converstion for generateMipmap on more platforms in the future.
On 2016/10/17 18:29:43, Zhenyao Mo wrote: > On 2016/10/15 15:14:39, yunchao wrote: > > On 2016/10/15 02:53:47, Zhenyao Mo wrote: > > > On 2016/10/15 01:11:38, yunchao wrote: > > > > On 2016/10/14 17:13:51, Zhenyao Mo wrote: > > > > > On 2016/10/14 07:28:34, yunchao wrote: > > > > > > Thanks for your quick review, Zhenyao. Please see the comments inline. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > > > > > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2420813002/diff/1/gpu/command_buffer/service/... > > > > > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:361: > > > > > > glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA32F, width, height, 0, GL_RGBA, > > > > > > On 2016/10/14 07:05:04, Zhenyao Mo wrote: > > > > > > > RGBA32F isn't color renderable or texture filterable by default, so > a > > > > bunch > > > > > of > > > > > > > calls, including generateMipmap will fail. You have to do either > > > > extension > > > > > > > check (I don't know the desktop situation - you should read the spec > > > > there), > > > > > > or > > > > > > > use the color_renderable and texture_filterable validators to make > > sure. > > > > > > > > > > > > In the OGL spec, RGBA32F is color-renderable (although it is not > > > > > > color-renderable in GLES spec). But there is no words about > > > > > texture-filterable, > > > > > > but it is an effective texture internalformat. I think it is texture > > > > > filterable > > > > > > now that no words says it can not to be. > > > > > > > > > > > > We tested the conformance test with this patch both on Linux mesa 3.3 > > and > > > > Mac > > > > > > Intel (OGL 4.1), it can pass without any error. > > > > > > > > > > > > Now that it can be support by default. I suppose it is OK without > extra > > > > check. > > > > > > What do you think? > > > > > > > > > > This can still fail on ES3 without EXT_color_buffer_float extension. > > > > > > > > > > Basically you will want to find a way to check if > > > > > FeatureInfo::ext_color_buffer_float_available_ is true here. > > > > > If not, RGBA8 is your fallback. > > > > > > > > Hi, Zhenyao, per my understanding, the code will be executed only in > OpenGL > > > > profile. > > > > In OpenGL, RGBA32F can be support directly, just like RGBA8 can be support > > > > directly. > > > > However, it is not necessary to emulate srgb on ES3. The emulation is not > > > > necessary in ES3 profile. > > > > So I still think it is not necessary to check and fallback. > > > > > > > > Maybe I am wrong, please correct me if so. > > > > > > This is for generateMipmap, right? I think even ES3 needs that workaround. > Am > > I > > > wrong? > > > > Zhenyao, your feedback reminded me that we should use high precision texture > > format for srgb conversion for blitFramebuffer too. Could you also take a look > > at https://codereview.chromium.org/2419153003/. For blitFramebuffer, it is > > definitely only for OGL profile, not for ES3 profile. The ES3 spec explicitly > > says it can support srgb conversion during blitFramebuffer without any > > emulation. > > > > Per generateMipmap, it is a little complicated. Both the OGL spec and GLES > spec > > don't say whether it can support srgb conversion during scaling. But the > actual > > tests show that: if blitFramebuffer can convert srgb during blitting, > > generateMipmap can convert srgb too (on OGL 4.4 and above), otherwise, if > > blitFramebuffer can not handle srgb conversion, generateMipmap will not handle > > srgb conversion directly during scaling. It is reasonable for driver > > implementation: convert srgb or not for all related APIs. So I suppose that on > > GLES3, it can convert srgb directly for generateMipmap without emulation too. > > And this patch (https://codereview.chromium.org/2318313004/) only add > workaround > > to emulate srgb for generateMipmap API for OGL profile, on which RGBA32F can > be > > support directly. > > > > Maybe that behavior is not correct. Maybe srgb emulation for generateMipmap is > > necessary for GLES3 profile too. > > So, should we verify both this fix and > > https://codereview.chromium.org/2318313004/ on more device with ES3 profile? > > I can test on one of the ES3 drivers and see if generateMipmap with sRGB works > there. > > The reason we want to be more careful here is we don't want to leak a GL error > in the future (who knows which platforms we may turn on this workaround in the > future). @Zhenyao @yunchao Thanks for your review. I have added check for ext_color_buffer_float_available_. As it's a private member of FeatureInfo, I also add a public function to return it. PTAL.
On 2016/10/14 06:35:56, yunchao wrote: > On 2016/10/14 06:29:52, yunchao wrote: > > LGTM. > > @zmo and all, PTAL. > > Zhenyao and Ken, could you grant mailto:Yizhou(yizhou.jiang@intel.com) the privilege to > run bots? When she try to run bots in trybots, or click CQ dry run, the bots > will fail to run. I suppose that she is not granted the privilege. > > Thanks in advance. Requested tryjob access for Yizhou. Will let you know once it's granted. Thanks for asking.
On 2016/10/19 23:55:23, Ken Russell wrote: > On 2016/10/14 06:35:56, yunchao wrote: > > On 2016/10/14 06:29:52, yunchao wrote: > > > LGTM. > > > @zmo and all, PTAL. > > > > Zhenyao and Ken, could you grant mailto:Yizhou(yizhou.jiang@intel.com) the > privilege to > > run bots? When she try to run bots in trybots, or click CQ dry run, the bots > > will fail to run. I suppose that she is not granted the privilege. > > > > Thanks in advance. > > Requested tryjob access for Yizhou. Will let you know once it's granted. Thanks > for asking. Thank you very much, Ken!
https://codereview.chromium.org/2420813002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2420813002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:364: if (feature_info_->ext_color_buffer_float_available()) { Sorry I forgot to mention this in earlier review, but you will also need oes_texture_float_linear_available_ in order to call generateMipmap on RGBA32F textures.
Thanks for your review @zmo, @kbr. Code has been revised, please have another look. https://codereview.chromium.org/2420813002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2420813002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:364: if (feature_info_->ext_color_buffer_float_available()) { On 2016/10/20 16:54:04, Zhenyao Mo wrote: > Sorry I forgot to mention this in earlier review, but you will also need > oes_texture_float_linear_available_ in order to call generateMipmap on RGBA32F > textures. Done.
On 2016/10/21 03:25:26, yizhou.jiang wrote: > Thanks for your review @zmo, @kbr. Code has been revised, please have another > look. > > https://codereview.chromium.org/2420813002/diff/40001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > https://codereview.chromium.org/2420813002/diff/40001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:364: if > (feature_info_->ext_color_buffer_float_available()) { > On 2016/10/20 16:54:04, Zhenyao Mo wrote: > > Sorry I forgot to mention this in earlier review, but you will also need > > oes_texture_float_linear_available_ in order to call generateMipmap on RGBA32F > > textures. > > Done. lgtm
I defer to zmo's review.
The CQ bit was checked by yizhou.jiang@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 yizhou.jiang@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 yizhou.jiang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from yunchao.he@intel.com, zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2420813002/#ps80001 (title: "rebase code")
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.
Description was changed from ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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 ========== optimize precision for conversion from srgb to linear Convertion from sRGB to 8-bit linear is going to reduce the precision of the texture, and the downsampling will do the same thing. We should set texture internalformat to rgba32f to optimize precision. BUG=634519 TEST=conformance2/textures/misc/tex-srgb-mipmap.html 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/0500571c0a41d977f64653eeaed4809351205a35 Cr-Commit-Position: refs/heads/master@{#427014} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0500571c0a41d977f64653eeaed4809351205a35 Cr-Commit-Position: refs/heads/master@{#427014} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
