Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2458103002:
Set correct internalformat info for TexStorageEXT according context type (Closed)
DescriptionSet correct internalformat info for TexStorageEXT according context type
For ES2 context, EXT_texture_storage enables TexStorageEXT. In the
extension spec, it says for OpenGL ES
"
The TexImage* calls in the TexStorage* pseudocode are modified so that
the <internalformat>, <format> and <type> parameters are taken from the
<format>, <format> and <type> columns (respectively).
"
See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt.
So, when we set texture level info, internalformat should be adjusted to
format in es2 context.
BUG=535198
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/12cc0e01ea62888ed2b84b3aeda6293194c819d4
Cr-Commit-Position: refs/heads/master@{#430501}
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase only #Patch Set 3 : use RGBA8_OES for ES2 #
Messages
Total messages: 28 (7 generated)
Description was changed from ========== Set correct internalformat info for TexStorageEXT according context type For ES2 context, EXT_texture_storage enables TexStorageEXT. In the extension spec, it says for OpenGL ES " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. So, when we set texture level info, internalformat should be adjusted to format in es2 context. BUG=535198 ========== to ========== Set correct internalformat info for TexStorageEXT according context type For ES2 context, EXT_texture_storage enables TexStorageEXT. In the extension spec, it says for OpenGL ES " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. So, when we set texture level info, internalformat should be adjusted to format in es2 context. BUG=535198 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 ==========
qiankun.miao@intel.com changed reviewers: + dongseong.hwang@intel.com, kbr@chromium.org, zmo@chromium.org
PTAL.
I'd like to defer review of this change to zmo@. CC'ing piman@ in case zmo@ is swamped.
I think dshwang purposely tried to set internalformat correctly (and ran into a Mac issue which could be a bug on our side in Mac path) and this CL reverts that effort.
On 2016/10/28 19:07:10, Zhenyao Mo wrote: > I think dshwang purposely tried to set internalformat correctly (and ran into a > Mac issue which could be a bug on our side in Mac path) and this CL reverts that > effort. I don't think it's the right fix for the Mac issue. The Mac issue is caused by another problem, see https://codereview.chromium.org/2458423002/.
On 2016/10/31 16:17:11, qiankun wrote: > On 2016/10/28 19:07:10, Zhenyao Mo wrote: > > I think dshwang purposely tried to set internalformat correctly (and ran into > a > > Mac issue which could be a bug on our side in Mac path) and this CL reverts > that > > effort. > > I don't think it's the right fix for the Mac issue. The Mac issue is caused by > another problem, see https://codereview.chromium.org/2458423002/. Your https://codereview.chromium.org/2458423002/ looks right fix. My fix is temporary because we didn't know why Mac fails. You find the root cause. Could you close this CL and do additional change to https://codereview.chromium.org/2458423002/? The another CL should remove #if defined(OS_MACOSX) block as the CL fix the root issue.
In addition, " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " explain about pseudocode. Even in ES2 context, TexStorage* must get sized format as internal format, so this CL is wrong.
https://codereview.chromium.org/2458103002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/2458103002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:16652: #endif I agree with Dongseong. Now you found the root problem in Mac, you can just remove this OS_MACOSX special hack, and get rid of the adjusted_format, and simply use internal_format below.
On 2016/10/31 16:56:41, dshwang wrote: > In addition, > " > The TexImage* calls in the TexStorage* pseudocode are modified so that > the <internalformat>, <format> and <type> parameters are taken from the > <format>, <format> and <type> columns (respectively). > " > explain about pseudocode. > > Even in ES2 context, TexStorage* must get sized format as internal format, so > this CL is wrong. https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt TexStorage* get sized format as internal format, but the result format and type of the texture is indicated by the pseudo-code: " Each of the commands below is described by pseudo-code which indicates the effect on the dimensions and format of the texture. For all of the commands, the following apply in addition to the pseudo-code: " According the above description, I think result texture and error message of TexStorage* are same as the pseudo-code.
On 2016/11/01 05:44:40, qiankun wrote: > On 2016/10/31 16:56:41, dshwang wrote: > > In addition, > > " > > The TexImage* calls in the TexStorage* pseudocode are modified so that > > the <internalformat>, <format> and <type> parameters are taken from the > > <format>, <format> and <type> columns (respectively). > > " > > explain about pseudocode. > > > > Even in ES2 context, TexStorage* must get sized format as internal format, so > > this CL is wrong. > > https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt > TexStorage* get sized format as internal format, but the result format and type > of the texture is indicated by the pseudo-code: > " > Each of the commands below is described by pseudo-code which > indicates the effect on the dimensions and format of the texture. > For all of the commands, the following apply in addition to the > pseudo-code: > " > > According the above description, I think result texture and error message of > TexStorage* are same as the pseudo-code. If calling glTexStorage2D(GL_RGBA) on EGL context directly, it fails. I believe chromium gpu cmd decoder should fail like real GL context.
On 2016/11/01 17:51:15, dshwang wrote: > On 2016/11/01 05:44:40, qiankun wrote: > > On 2016/10/31 16:56:41, dshwang wrote: > > > In addition, > > > " > > > The TexImage* calls in the TexStorage* pseudocode are modified so that > > > the <internalformat>, <format> and <type> parameters are taken from the > > > <format>, <format> and <type> columns (respectively). > > > " > > > explain about pseudocode. > > > > > > Even in ES2 context, TexStorage* must get sized format as internal format, > so > > > this CL is wrong. > > > > https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt > > TexStorage* get sized format as internal format, but the result format and > type > > of the texture is indicated by the pseudo-code: > > " > > Each of the commands below is described by pseudo-code which > > indicates the effect on the dimensions and format of the texture. > > For all of the commands, the following apply in addition to the > > pseudo-code: > > " > > > > According the above description, I think result texture and error message of > > TexStorage* are same as the pseudo-code. > > If calling glTexStorage2D(GL_RGBA) on EGL context directly, it fails. I believe > chromium gpu cmd decoder should fail like real GL context. Calling glTexStorage2D(GL_RGBA) should fail because glTexStorage2D only accepts sized internal formats. What I fixed in this CL is for ES2 context when calling TexStorage2DEXT(GL_RGBA8) under GL_ARB_texture_storage extension, we should set internal_format texture info as RGBA. See the gpu_unittests added for ES2 and ES3.
On 2016/11/02 02:43:34, qiankun wrote: > On 2016/11/01 17:51:15, dshwang wrote: > > On 2016/11/01 05:44:40, qiankun wrote: > > > On 2016/10/31 16:56:41, dshwang wrote: > > > > In addition, > > > > " > > > > The TexImage* calls in the TexStorage* pseudocode are modified so that > > > > the <internalformat>, <format> and <type> parameters are taken from the > > > > <format>, <format> and <type> columns (respectively). > > > > " > > > > explain about pseudocode. > > > > > > > > Even in ES2 context, TexStorage* must get sized format as internal format, > > so > > > > this CL is wrong. > > > > > > https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt > > > TexStorage* get sized format as internal format, but the result format and > > type > > > of the texture is indicated by the pseudo-code: > > > " > > > Each of the commands below is described by pseudo-code which > > > indicates the effect on the dimensions and format of the texture. > > > For all of the commands, the following apply in addition to the > > > pseudo-code: > > > " > > > > > > According the above description, I think result texture and error message of > > > TexStorage* are same as the pseudo-code. > > > > If calling glTexStorage2D(GL_RGBA) on EGL context directly, it fails. I > believe > > chromium gpu cmd decoder should fail like real GL context. > > Calling glTexStorage2D(GL_RGBA) should fail because glTexStorage2D only accepts > sized internal formats. > What I fixed in this CL is for ES2 context when calling > TexStorage2DEXT(GL_RGBA8) under GL_ARB_texture_storage extension, we should set > internal_format texture info as RGBA. See the gpu_unittests added for ES2 and > ES3. Why do we should set internal_format texture info as RGBA? GPU driver set internal_format texture info as RGBA8 tho. IMO, this inconsistency adds unnecessary maintenance burden.
On Wed, Nov 2, 2016 at 6:05 AM, <dongseong.hwang@intel.com> wrote: > On 2016/11/02 02:43:34, qiankun wrote: > > On 2016/11/01 17:51:15, dshwang wrote: > > > On 2016/11/01 05:44:40, qiankun wrote: > > > > On 2016/10/31 16:56:41, dshwang wrote: > > > > > In addition, > > > > > " > > > > > The TexImage* calls in the TexStorage* pseudocode are modified so > that > > > > > the <internalformat>, <format> and <type> parameters are taken > from the > > > > > <format>, <format> and <type> columns (respectively). > > > > > " > > > > > explain about pseudocode. > > > > > > > > > > Even in ES2 context, TexStorage* must get sized format as internal > format, > > > so > > > > > this CL is wrong. > > > > > > > > > https://www.khronos.org/registry/gles/extensions/EXT/ > EXT_texture_storage.txt > > > > TexStorage* get sized format as internal format, but the result > format and > > > type > > > > of the texture is indicated by the pseudo-code: > > > > " > > > > Each of the commands below is described by pseudo-code which > > > > indicates the effect on the dimensions and format of the texture. > > > > For all of the commands, the following apply in addition to the > > > > pseudo-code: > > > > " > > > > > > > > According the above description, I think result texture and error > message > of > > > > TexStorage* are same as the pseudo-code. > > > > > > If calling glTexStorage2D(GL_RGBA) on EGL context directly, it fails. I > > believe > > > chromium gpu cmd decoder should fail like real GL context. > > > > Calling glTexStorage2D(GL_RGBA) should fail because glTexStorage2D only > accepts > > sized internal formats. > > What I fixed in this CL is for ES2 context when calling > > TexStorage2DEXT(GL_RGBA8) under GL_ARB_texture_storage extension, we > should > set > > internal_format texture info as RGBA. See the gpu_unittests added for > ES2 and > > ES3. > > Why do we should set internal_format texture info as RGBA? GPU driver set > internal_format texture info as RGBA8 tho. IMO, this inconsistency adds > unnecessary maintenance burden. > It is a burden, and a bit of a mess. However it is needed currently to ensure correct handling of texture completeness - e.g. if one cube map face is setup with TexStorage2D(GL_RGBA8) and other faces setup with TexImage2D with GL_RGBA as the internal format (needed by ES2), then the texture should be complete, but that needs the internal formats to match. Keep in mind that the data in the ImageInfor/LevelInfo represents the API state (as considered by the command buffer), not what we gave to the underlying driver. See crbug.com/628064 for some details of how we would need to change the ImageInfo/LevelInfo to be consistent in both ES2 and ES3. It's a bit of a big refactoring. > > https://codereview.chromium.org/2458103002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/02 18:19:38, piman wrote: > On Wed, Nov 2, 2016 at 6:05 AM, <mailto:dongseong.hwang@intel.com> wrote: > > > On 2016/11/02 02:43:34, qiankun wrote: > > > On 2016/11/01 17:51:15, dshwang wrote: > > > > On 2016/11/01 05:44:40, qiankun wrote: > > > > > On 2016/10/31 16:56:41, dshwang wrote: > > > > > > In addition, > > > > > > " > > > > > > The TexImage* calls in the TexStorage* pseudocode are modified so > > that > > > > > > the <internalformat>, <format> and <type> parameters are taken > > from the > > > > > > <format>, <format> and <type> columns (respectively). > > > > > > " > > > > > > explain about pseudocode. > > > > > > > > > > > > Even in ES2 context, TexStorage* must get sized format as internal > > format, > > > > so > > > > > > this CL is wrong. > > > > > > > > > > > > https://www.khronos.org/registry/gles/extensions/EXT/ > > EXT_texture_storage.txt > > > > > TexStorage* get sized format as internal format, but the result > > format and > > > > type > > > > > of the texture is indicated by the pseudo-code: > > > > > " > > > > > Each of the commands below is described by pseudo-code which > > > > > indicates the effect on the dimensions and format of the texture. > > > > > For all of the commands, the following apply in addition to the > > > > > pseudo-code: > > > > > " > > > > > > > > > > According the above description, I think result texture and error > > message > > of > > > > > TexStorage* are same as the pseudo-code. > > > > > > > > If calling glTexStorage2D(GL_RGBA) on EGL context directly, it fails. I > > > believe > > > > chromium gpu cmd decoder should fail like real GL context. > > > > > > Calling glTexStorage2D(GL_RGBA) should fail because glTexStorage2D only > > accepts > > > sized internal formats. > > > What I fixed in this CL is for ES2 context when calling > > > TexStorage2DEXT(GL_RGBA8) under GL_ARB_texture_storage extension, we > > should > > set > > > internal_format texture info as RGBA. See the gpu_unittests added for > > ES2 and > > > ES3. > > > > Why do we should set internal_format texture info as RGBA? GPU driver set > > internal_format texture info as RGBA8 tho. IMO, this inconsistency adds > > unnecessary maintenance burden. > > > > It is a burden, and a bit of a mess. However it is needed currently to > ensure correct handling of texture completeness - e.g. if one cube map face > is setup with TexStorage2D(GL_RGBA8) and other faces setup with TexImage2D > with GL_RGBA as the internal format (needed by ES2), then the texture > should be complete, but that needs the internal formats to match. Keep in > mind that the data in the ImageInfor/LevelInfo represents the API state (as > considered by the command buffer), not what we gave to the underlying > driver. > See crbug.com/628064 for some details of how we would need to change the > ImageInfo/LevelInfo to be consistent in both ES2 and ES3. It's a bit of a > big refactoring. Oh, thank you for explaining. So this CL is needed eventually. Do you think it should be landed now or after introducing ImageInfo? All I want is to make CMAA impl know exact internal_format. I'm happy with this CL only if ImageInfo give the information.
On Wed, Nov 2, 2016 at 12:12 PM, <dongseong.hwang@intel.com> wrote: > On 2016/11/02 18:19:38, piman wrote: > > > On Wed, Nov 2, 2016 at 6:05 AM, <mailto:dongseong.hwang@intel.com> > wrote: > > > > > On 2016/11/02 02:43:34, qiankun wrote: > > > > On 2016/11/01 17:51:15, dshwang wrote: > > > > > On 2016/11/01 05:44:40, qiankun wrote: > > > > > > On 2016/10/31 16:56:41, dshwang wrote: > > > > > > > In addition, > > > > > > > " > > > > > > > The TexImage* calls in the TexStorage* pseudocode are modified > so > > > that > > > > > > > the <internalformat>, <format> and <type> parameters are taken > > > from the > > > > > > > <format>, <format> and <type> columns (respectively). > > > > > > > " > > > > > > > explain about pseudocode. > > > > > > > > > > > > > > Even in ES2 context, TexStorage* must get sized format as > internal > > > format, > > > > > so > > > > > > > this CL is wrong. > > > > > > > > > > > > > > > https://www.khronos.org/registry/gles/extensions/EXT/ > > > EXT_texture_storage.txt > > > > > > TexStorage* get sized format as internal format, but the result > > > format and > > > > > type > > > > > > of the texture is indicated by the pseudo-code: > > > > > > " > > > > > > Each of the commands below is described by pseudo-code which > > > > > > indicates the effect on the dimensions and format of the texture. > > > > > > For all of the commands, the following apply in addition to the > > > > > > pseudo-code: > > > > > > " > > > > > > > > > > > > According the above description, I think result texture and error > > > message > > > of > > > > > > TexStorage* are same as the pseudo-code. > > > > > > > > > > If calling glTexStorage2D(GL_RGBA) on EGL context directly, it > fails. I > > > > believe > > > > > chromium gpu cmd decoder should fail like real GL context. > > > > > > > > Calling glTexStorage2D(GL_RGBA) should fail because glTexStorage2D > only > > > accepts > > > > sized internal formats. > > > > What I fixed in this CL is for ES2 context when calling > > > > TexStorage2DEXT(GL_RGBA8) under GL_ARB_texture_storage extension, we > > > should > > > set > > > > internal_format texture info as RGBA. See the gpu_unittests added for > > > ES2 and > > > > ES3. > > > > > > Why do we should set internal_format texture info as RGBA? GPU driver > set > > > internal_format texture info as RGBA8 tho. IMO, this inconsistency adds > > > unnecessary maintenance burden. > > > > > > > It is a burden, and a bit of a mess. However it is needed currently to > > ensure correct handling of texture completeness - e.g. if one cube map > face > > is setup with TexStorage2D(GL_RGBA8) and other faces setup with > TexImage2D > > with GL_RGBA as the internal format (needed by ES2), then the texture > > should be complete, but that needs the internal formats to match. Keep in > > mind that the data in the ImageInfor/LevelInfo represents the API state > (as > > considered by the command buffer), not what we gave to the underlying > > driver. > > See crbug.com/628064 for some details of how we would need to change the > > ImageInfo/LevelInfo to be consistent in both ES2 and ES3. It's a bit of a > > big refactoring. > > Oh, thank you for explaining. So this CL is needed eventually. > Do you think it should be landed now or after introducing ImageInfo? > Apologies, my description was confusing. I meant "FaceInfo" instead of "ImageInfo" - it's already existing, nothing to introduce. That bug explains how we would modify their contents to consistently support the API (in practice: remove format and type, and make internal_format always be the effective_internal_format, i.e. sized, from which we can derive back type/format as needed). > All I want is to make CMAA impl know exact internal_format. I'm happy with > this > CL only if ImageInfo give the information. > What do you mean by "exact internal_format"? There's several possible meaningful internal formats: - the unsized internal format passed to the command buffer API (e.g. via glTexImage2D) - the effective internal format as would be computed by the ES3 semantics (i.e. sized based on format/type - more or less what GLES2Util::ConvertToSizedFormat does) - the internal format we pass to the underlying driver (the result of TextureManager::AdjustTexInternalFormat) They may all be different - for example the API would accept glTexImage2D(..., GL_LUMINANCE, ..., GL_LUMINANCE, GL_FLOAT, ...), i.e. with an API-level internal format of GL_LUMINANCE. This would become a GL_LUMINANCE32F effective internal format, that we would translate into GL_R32F when calling an underlying driver that uses the core profile. Ideally all semantic cross-validation checks would be on the effective internal format - that's effectively what the spec mandates. But we currently don't track it and that's what crbug.com/628064 is about. We effectively have a proxy by keeping all of (unsized)internal_format/format/type currently, as an ES2 "mostly" equivalent representation. > https://codereview.chromium.org/2458103002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Set correct internalformat info for TexStorageEXT according context type For ES2 context, EXT_texture_storage enables TexStorageEXT. In the extension spec, it says for OpenGL ES " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. So, when we set texture level info, internalformat should be adjusted to format in es2 context. BUG=535198 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 ========== Set correct internalformat info for TexStorageEXT according context type For ES2 context, EXT_texture_storage enables TexStorageEXT. In the extension spec, it says for OpenGL ES " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. So, when we set texture level info, internalformat should be adjusted to format in es2 context. BUG=535198 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 ==========
qiankun.miao@intel.com changed reviewers: + piman@chromium.org
On 2016/11/02 18:19:38, piman wrote: > On Wed, Nov 2, 2016 at 6:05 AM, <mailto:dongseong.hwang@intel.com> wrote: > > > On 2016/11/02 02:43:34, qiankun wrote: > > > On 2016/11/01 17:51:15, dshwang wrote: > > > > On 2016/11/01 05:44:40, qiankun wrote: > > > > > On 2016/10/31 16:56:41, dshwang wrote: > > > > > > In addition, > > > > > > " > > > > > > The TexImage* calls in the TexStorage* pseudocode are modified so > > that > > > > > > the <internalformat>, <format> and <type> parameters are taken > > from the > > > > > > <format>, <format> and <type> columns (respectively). > > > > > > " > > > > > > explain about pseudocode. > > > > > > > > > > > > Even in ES2 context, TexStorage* must get sized format as internal > > format, > > > > so > > > > > > this CL is wrong. > > > > > > > > > > > > https://www.khronos.org/registry/gles/extensions/EXT/ > > EXT_texture_storage.txt > > > > > TexStorage* get sized format as internal format, but the result > > format and > > > > type > > > > > of the texture is indicated by the pseudo-code: > > > > > " > > > > > Each of the commands below is described by pseudo-code which > > > > > indicates the effect on the dimensions and format of the texture. > > > > > For all of the commands, the following apply in addition to the > > > > > pseudo-code: > > > > > " > > > > > > > > > > According the above description, I think result texture and error > > message > > of > > > > > TexStorage* are same as the pseudo-code. > > > > > > > > If calling glTexStorage2D(GL_RGBA) on EGL context directly, it fails. I > > > believe > > > > chromium gpu cmd decoder should fail like real GL context. > > > > > > Calling glTexStorage2D(GL_RGBA) should fail because glTexStorage2D only > > accepts > > > sized internal formats. > > > What I fixed in this CL is for ES2 context when calling > > > TexStorage2DEXT(GL_RGBA8) under GL_ARB_texture_storage extension, we > > should > > set > > > internal_format texture info as RGBA. See the gpu_unittests added for > > ES2 and > > > ES3. > > > > Why do we should set internal_format texture info as RGBA? GPU driver set > > internal_format texture info as RGBA8 tho. IMO, this inconsistency adds > > unnecessary maintenance burden. > > > > It is a burden, and a bit of a mess. However it is needed currently to > ensure correct handling of texture completeness - e.g. if one cube map face > is setup with TexStorage2D(GL_RGBA8) and other faces setup with TexImage2D > with GL_RGBA as the internal format (needed by ES2), then the texture > should be complete, but that needs the internal formats to match. Keep in > mind that the data in the ImageInfor/LevelInfo represents the API state (as > considered by the command buffer), not what we gave to the underlying > driver. > See crbug.com/628064 for some details of how we would need to change the > ImageInfo/LevelInfo to be consistent in both ES2 and ES3. It's a bit of a > big refactoring. In fact, we cannot setup other faces of a texture with TexImage2D after TexStorage2DEXT since the texture is immutable. We only can update texture data with TexSubImage2D. In ES2, TexSubImage2D requires format matches internalformat specified with TexStorage2DEXT. That's why we need this change. In chrome both ES2 and ES3 context requires valid internalformat/format/type combination for TexImage* and TexSubImage*, see https://cs.chromium.org/chromium/src/gpu/command_buffer/service/texture_manag... . Internalformat for TexSubImage2D is queried from texture info which was set when doing TexStorage2D or TexImage2D.
Any further comments about this CL? Can we land it?
On 2016/11/07 10:52:21, qiankun wrote: > Any further comments about this CL? Can we land it? lgtm per the above discussion
On 2016/11/07 23:15:53, Zhenyao Mo wrote: > On 2016/11/07 10:52:21, qiankun wrote: > > Any further comments about this CL? Can we land it? > > lgtm per the above discussion Thank you all for review:)
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...
Message was sent while issue was closed.
Description was changed from ========== Set correct internalformat info for TexStorageEXT according context type For ES2 context, EXT_texture_storage enables TexStorageEXT. In the extension spec, it says for OpenGL ES " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. So, when we set texture level info, internalformat should be adjusted to format in es2 context. BUG=535198 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 ========== Set correct internalformat info for TexStorageEXT according context type For ES2 context, EXT_texture_storage enables TexStorageEXT. In the extension spec, it says for OpenGL ES " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. So, when we set texture level info, internalformat should be adjusted to format in es2 context. BUG=535198 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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Set correct internalformat info for TexStorageEXT according context type For ES2 context, EXT_texture_storage enables TexStorageEXT. In the extension spec, it says for OpenGL ES " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. So, when we set texture level info, internalformat should be adjusted to format in es2 context. BUG=535198 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 ========== Set correct internalformat info for TexStorageEXT according context type For ES2 context, EXT_texture_storage enables TexStorageEXT. In the extension spec, it says for OpenGL ES " The TexImage* calls in the TexStorage* pseudocode are modified so that the <internalformat>, <format> and <type> parameters are taken from the <format>, <format> and <type> columns (respectively). " See https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_storage.txt. So, when we set texture level info, internalformat should be adjusted to format in es2 context. BUG=535198 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/12cc0e01ea62888ed2b84b3aeda6293194c819d4 Cr-Commit-Position: refs/heads/master@{#430501} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/12cc0e01ea62888ed2b84b3aeda6293194c819d4 Cr-Commit-Position: refs/heads/master@{#430501} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
