Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1547873002:
Allow GL_TEXTURE_RECTANGLE_ARB to be the destination target for copyTextureCHROMIUM. (Closed)
|
Created:
5 years ago by erikchen Modified:
4 years, 11 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, piman+watch_chromium.org, extensions-reviews_chromium.org, vmiura Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow GL_TEXTURE_RECTANGLE_ARB as the destination for copyTextureCHROMIUM.
I rewrote the vertex shader to support the new destination target. In the
process, I recalculated the transforms for the vertex shader, and fixed mistakes
in the original shader (it functioned incorrectly when copying sub-textures).
BUG=533617
Committed: https://crrev.com/6b91a5080f1ca2a3c95b1ee9308f6884b33dcd13
Cr-Commit-Position: refs/heads/master@{#367920}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : Fix android compile error. #Patch Set 8 : Test fixes. #Patch Set 9 : More test fixes. #Patch Set 10 : Fix shader compile error. #
Total comments: 7
Patch Set 11 : Rebase against https://codereview.chromium.org/1551143002/. #
Total comments: 8
Patch Set 12 : Comments from zmo. #
Total comments: 10
Patch Set 13 : Comments from zmo. #Patch Set 14 : Fix test. #Patch Set 15 : Remove changes to decoder unittest. #
Total comments: 6
Patch Set 16 : Comments from kbr. #Patch Set 17 : Comments from kbr. #
Depends on Patchset: Messages
Total messages: 65 (23 generated)
Description was changed from ========== Allow GL_TEXTURE_RECTANGLE_ARB to be the destination target for copyTextureCHROMIUM. starting to work. BUG= ========== to ========== Allow GL_TEXTURE_RECTANGLE_ARB as the destination for copyTextureCHROMIUM. I rewrote the vertex shader to support the new destination target. In the process, I recalculated the transforms for the vertex shader, and fixed mistakes in the original shader (it functioned incorrectly when copying sub-textures). BUG= ==========
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/patch-status/1547873002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547873002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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/patch-status/1547873002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547873002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/patch-status/1547873002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547873002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/patch-status/1547873002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547873002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/patch-status/1547873002/100002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547873002/100002
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Allow GL_TEXTURE_RECTANGLE_ARB as the destination for copyTextureCHROMIUM. I rewrote the vertex shader to support the new destination target. In the process, I recalculated the transforms for the vertex shader, and fixed mistakes in the original shader (it functioned incorrectly when copying sub-textures). BUG= ========== to ========== Allow GL_TEXTURE_RECTANGLE_ARB as the destination for copyTextureCHROMIUM. I rewrote the vertex shader to support the new destination target. In the process, I recalculated the transforms for the vertex shader, and fixed mistakes in the original shader (it functioned incorrectly when copying sub-textures). BUG=533617 ==========
erikchen@chromium.org changed reviewers: + kbr@chromium.org
kbr: Please review. https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (left): https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1933: // [Compressed]Tex[Sub]Image2D and related functions. I removed this to get my unit test to work. It's not clear to me why this is necessary and correct, even when reading the the CL where it was introduced: https://codereview.chromium.org/1376633002/ Perhaps someone with more experience could comment?
kbr@chromium.org changed reviewers: + zmo@chromium.org
+zmo for OWNERS review
On 2015/12/30 22:09:53, Ken Russell wrote: > +zmo for OWNERS review This CL generally looks good. Eric, you did a terrific job. However, while reviewing this CL, I got very confused with a bunch of things with the extension spec. In general, why do we specify |target| for all Copy* functions? Since we specify an already created and bound dest texture, we won't be to rebind that texture. So should we just get rid of the |target| arg for all these functions, and use dest texture's target instead? This affects a bunch of places in this CL. I'd like @piman (or whoever is familiar with this extension) to clarify before giving the green signal. Eric, sorry it's not a problem caused by your CL, but now I am reviewing it, I will need the answers.
zmo@chromium.org changed reviewers: + piman@chromium.org
On 2015/12/30 23:23:11, Zhenyao Mo wrote: > On 2015/12/30 22:09:53, Ken Russell wrote: > > +zmo for OWNERS review > > This CL generally looks good. Eric, you did a terrific job. > > However, while reviewing this CL, I got very confused with a bunch of things > with the extension spec. > > In general, why do we specify |target| for all Copy* functions? Since we specify > an already created and bound dest texture, we won't be to rebind that texture. > So should we just get rid of the |target| arg for all these functions, and use > dest texture's target instead? > > This affects a bunch of places in this CL. Unfortunately it's not possible to query a texture object's target (i.e., the first binding point it was bound to) until OpenGL 4.5. See https://www.opengl.org/sdk/docs/man/html/glGetTexParameter.xhtml and the GL_TEXTURE_TARGET query parameter. For this reason it seems necessary to pass the destination texture's target down to the lower-level code, after it's queried from the command buffer's TextureManager and Texture. > I'd like @piman (or whoever is familiar with this extension) to clarify before > giving the green signal. > > Eric, sorry it's not a problem caused by your CL, but now I am reviewing it, I > will need the answers.
https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13280: target, 0, &dest_width, &dest_height, nullptr); Is the intent of this CL to allow copying between TEXTURE_2D and TEXTURE_RECTANGLE_ARB textures? If so, it seems to me this should reference the destination texture's target. If the intent's to only support copying between two textures of the same type, then there should probably be checks throughout. https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13283: dest_texture->GetLevelType(target, 0, &dest_type_previous, Same here; seems this should reference the destination texture's target. https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13295: glTexImage2D(target, 0, internal_format, source_width, source_height, These two calls seem to me like they should be using the destination texture's target, not the source texture's. It looks like the same issue exists below.
On 2015/12/31 02:11:58, Ken Russell wrote: > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:13280: target, 0, &dest_width, > &dest_height, nullptr); > Is the intent of this CL to allow copying between TEXTURE_2D and > TEXTURE_RECTANGLE_ARB textures? If so, it seems to me this should reference the > destination texture's target. > > If the intent's to only support copying between two textures of the same type, > then there should probably be checks throughout. > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:13283: > dest_texture->GetLevelType(target, 0, &dest_type_previous, > Same here; seems this should reference the destination texture's target. > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:13295: glTexImage2D(target, 0, > internal_format, source_width, source_height, > These two calls seem to me like they should be using the destination texture's > target, not the source texture's. It looks like the same issue exists below. I think all the reference to target should actually be dest texture's target. That's why we should get rid of the passed in "target" arg as it's useless and causes confusion and bugs.
On 2016/01/04 18:19:31, Zhenyao Mo wrote: > On 2015/12/31 02:11:58, Ken Russell wrote: > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:13280: target, 0, &dest_width, > > &dest_height, nullptr); > > Is the intent of this CL to allow copying between TEXTURE_2D and > > TEXTURE_RECTANGLE_ARB textures? If so, it seems to me this should reference > the > > destination texture's target. > > > > If the intent's to only support copying between two textures of the same type, > > then there should probably be checks throughout. > > > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:13283: > > dest_texture->GetLevelType(target, 0, &dest_type_previous, > > Same here; seems this should reference the destination texture's target. > > > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:13295: glTexImage2D(target, 0, > > internal_format, source_width, source_height, > > These two calls seem to me like they should be using the destination texture's > > target, not the source texture's. It looks like the same issue exists below. > > I think all the reference to target should actually be dest texture's target. > That's why we should get rid of the passed in "target" arg as it's useless and > causes confusion and bugs. If the function's supposed to support copying from both TEXTURE_2D and TEXTURE_RECTANGLE_ARB textures, then different shaders are needed to sample each of those input texture types, if the copy's done by rendering a textured quad to another texture. For that reason I think both the source and dest textures' targets are needed.
On 2016/01/04 21:15:31, Ken Russell wrote: > On 2016/01/04 18:19:31, Zhenyao Mo wrote: > > On 2015/12/31 02:11:58, Ken Russell wrote: > > > > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > > > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:13280: target, 0, > &dest_width, > > > &dest_height, nullptr); > > > Is the intent of this CL to allow copying between TEXTURE_2D and > > > TEXTURE_RECTANGLE_ARB textures? If so, it seems to me this should reference > > the > > > destination texture's target. > > > > > > If the intent's to only support copying between two textures of the same > type, > > > then there should probably be checks throughout. > > > > > > > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:13283: > > > dest_texture->GetLevelType(target, 0, &dest_type_previous, > > > Same here; seems this should reference the destination texture's target. > > > > > > > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:13295: glTexImage2D(target, > 0, > > > internal_format, source_width, source_height, > > > These two calls seem to me like they should be using the destination > texture's > > > target, not the source texture's. It looks like the same issue exists below. > > > > I think all the reference to target should actually be dest texture's target. > > That's why we should get rid of the passed in "target" arg as it's useless and > > causes confusion and bugs. > > If the function's supposed to support copying from both TEXTURE_2D and > TEXTURE_RECTANGLE_ARB textures, then different shaders are needed to sample each > of those input texture types, if the copy's done by rendering a textured quad to > another texture. For that reason I think both the source and dest textures' > targets are needed. I think there's a miscommunication here. zmo asked me to remove a redundant argument to CopyTextureChromium, which is already in review: https://codereview.chromium.org/1551143002/ He is not suggesting that we change the signature of DoCopyTexture().
zmo: PTAL. I rebased this CL against https://codereview.chromium.org/1551143002/, which makes the changes to gles2_cmd_decoder.cc much easier to read. https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13280: target, 0, &dest_width, &dest_height, nullptr); On 2015/12/31 02:11:58, Ken Russell wrote: > Is the intent of this CL to allow copying between TEXTURE_2D and > TEXTURE_RECTANGLE_ARB textures? If so, it seems to me this should reference the > destination texture's target. > > If the intent's to only support copying between two textures of the same type, > then there should probably be checks throughout. This CL allows copying from TEXTURE_2D/TEXTURE_RECTANGLE_ARB to TEXTURE_2D/TEXTURE_RECTANGLE_ARB. You're right - "target" was the wrong parameter to pass here. The CL that this CL depends on fixes the problem by getting rid of the "target" parameter entirely. https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13283: dest_texture->GetLevelType(target, 0, &dest_type_previous, On 2015/12/31 02:11:58, Ken Russell wrote: > Same here; seems this should reference the destination texture's target. yup, fixed. https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13295: glTexImage2D(target, 0, internal_format, source_width, source_height, On 2015/12/31 02:11:58, Ken Russell wrote: > These two calls seem to me like they should be using the destination texture's > target, not the source texture's. It looks like the same issue exists below. yup, fixed.
Mostly looks good with a few questions. https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:303: glBindTexture(GL_TEXTURE_2D, dest_id); Should you use dest_target here and in the following gl calls? https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13086: dest_texture->target() != GL_TEXTURE_RECTANGLE_ARB) || nit: maybe use two switch statements here? Looks nicer and you can have separate error msgs for src and dst targets. https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13853: glBindTexture(GL_TEXTURE_2D, tmp_service_id); I think it's clearer if you define a variable of intermediate_target and set it as TEXTURE_2D, and use it here and below. Much easier to understand the semantics that way. https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13868: glBindTexture(GL_TEXTURE_2D, dest_texture->service_id()); Here we have the assumption that dest_texture->target() is GL_TEXTURE_2D, so we probably should DCHECK right in the beginning? Or actually the tmp_service_id should be bound to dest_texture->target() instead of GL_TEXTURE_2D?
zmo: PTAL https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:303: glBindTexture(GL_TEXTURE_2D, dest_id); On 2016/01/04 22:43:54, Zhenyao Mo wrote: > Should you use dest_target here and in the following gl calls? This function assumes that dest_target is GL_TEXTURE_2D, which is enforced by the caller of the function. I removed the parameter dest_target, and kept using GL_TEXTURE_2D as the parameter to these functions. https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13086: dest_texture->target() != GL_TEXTURE_RECTANGLE_ARB) || On 2016/01/04 22:43:54, Zhenyao Mo wrote: > nit: maybe use two switch statements here? Looks nicer and you can have > separate error msgs for src and dst targets. Done. https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13853: glBindTexture(GL_TEXTURE_2D, tmp_service_id); On 2016/01/04 22:43:54, Zhenyao Mo wrote: > I think it's clearer if you define a variable of intermediate_target and set it > as TEXTURE_2D, and use it here and below. Much easier to understand the > semantics that way. Done. I used the name "tmp_target" to match "tmp_service_id". https://codereview.chromium.org/1547873002/diff/190001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13868: glBindTexture(GL_TEXTURE_2D, dest_texture->service_id()); On 2016/01/04 22:43:55, Zhenyao Mo wrote: > Here we have the assumption that dest_texture->target() is GL_TEXTURE_2D, so we > probably should DCHECK right in the beginning? Or actually the tmp_service_id > should be bound to dest_texture->target() instead of GL_TEXTURE_2D? I added a DCHECK to the beginning. Note that this is part of the implementation of DoCompressedCopySubTextureCHROMIUM(), which only supports GL_TEXTURE_2D as the destination target.
https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:270: DCHECK(source_target == GL_TEXTURE_2D); Can we also pass in dest_target and DCHECK it's TEXTURE_2D? and use dest_target below. This way we will make sure no one calls this function with a texture of wrong target. https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:291: GLuint dest_id, Same here, can we also pass in dest_target and DCHECK it's TEXTURE_2D? and use dest_target below. This way we will make sure no one calls this function with a texture of wrong target. https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13908: dest_texture->service_id(), GL_RGBA, xoffset, yoffset, x, y, width, OK, sorry for raising all the questions. This is me trying to fully understand the constraints. It seems like the dest_internal_format doesn't have to be GL_RGBA here, as we can pass it in as a parameter and I don't see a limitation in DoCopySubTexture. If this is the case, why do we have the above fallback if dets_internal_format != GL_RGBA?
https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (left): https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1939: } Your change seems unrelated to glTexImage2D and friends, where we still need to disable GL_TEXTURE_RECTANGLE_ARB as a target (IIUC because uploading software pixels to an HW-backed IOSurface causes trouble), so I think you need to keep this here.
https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (left): https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1939: } On 2016/01/05 01:36:39, piman (OOO until 2016-1-4) wrote: > Your change seems unrelated to glTexImage2D and friends, where we still need to > disable GL_TEXTURE_RECTANGLE_ARB as a target (IIUC because uploading software > pixels to an HW-backed IOSurface causes trouble), so I think you need to keep > this here. I commented on this in patch set 10: https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... Could you please clarify what code will call glTexImage2D() with GL_TEXTURE_RECTANGLE_ARB as a target on an IOSurface backed texture? I removed this check because the unit test I added needed to call glTexImage2D() with target=GL_TEXTURE_RECTANGLE_ARB on a non-IOSurface backed texture. If we need to keep this check, can you suggest how should I restructure the unit test?
https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:270: DCHECK(source_target == GL_TEXTURE_2D); On 2016/01/05 00:52:16, Zhenyao Mo wrote: > Can we also pass in dest_target and DCHECK it's TEXTURE_2D? and use dest_target > below. This way we will make sure no one calls this function with a texture of > wrong target. Done. https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:291: GLuint dest_id, On 2016/01/05 00:52:16, Zhenyao Mo wrote: > Same here, can we also pass in dest_target and DCHECK it's TEXTURE_2D? and use > dest_target below. This way we will make sure no one calls this function with a > texture of wrong target. Done. https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13908: dest_texture->service_id(), GL_RGBA, xoffset, yoffset, x, y, width, On 2016/01/05 00:52:16, Zhenyao Mo wrote: > OK, sorry for raising all the questions. This is me trying to fully understand > the constraints. It seems like the dest_internal_format doesn't have to be > GL_RGBA here, as we can pass it in as a parameter and I don't see a limitation > in DoCopySubTexture. If this is the case, why do we have the above fallback if > dets_internal_format != GL_RGBA? I moved this discussion to https://code.google.com/p/chromium/issues/detail?id=574278
+ccameron in case he has a better idea https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (left): https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1939: } On 2016/01/05 01:45:15, erikchen wrote: > On 2016/01/05 01:36:39, piman (OOO until 2016-1-4) wrote: > > Your change seems unrelated to glTexImage2D and friends, where we still need > to > > disable GL_TEXTURE_RECTANGLE_ARB as a target (IIUC because uploading software > > pixels to an HW-backed IOSurface causes trouble), so I think you need to keep > > this here. > > I commented on this in patch set 10: > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > Could you please clarify what code will call glTexImage2D() with > GL_TEXTURE_RECTANGLE_ARB as a target on an IOSurface backed texture? Actually, my recollection was slightly incorrect, the bad case is binding an IOSurface after using glTexImage2D. I think Flash would do that - see https://code.google.com/p/chromium/issues/detail?id=518744 - though it may have been updated since then. The decision was to disallow uploading texels altogether on a GL_TEXTURE_RECTANGLE_ARB texture, hence this code. But either way, the client is untrusted, we can't assume it will only do the right thing. > I removed this check because the unit test I added needed to call glTexImage2D() > with target=GL_TEXTURE_RECTANGLE_ARB on a non-IOSurface backed texture. If we > need to keep this check, can you suggest how should I restructure the unit test? My suggestion would be to model the test after the use case we have for GL_TEXTURE_RECTANGLE_ARB, namely binding an IOSurface, instead of uploading texels.
lgtm from my point of view but please wait for ccameron to take a look
On 2016/01/05 17:47:39, Zhenyao Mo wrote: > lgtm from my point of view > > but please wait for ccameron to take a look not lgtm because I don't see how this would not regress crbug.com/518744
On 2016/01/05 at 17:49:45, piman wrote: > On 2016/01/05 17:47:39, Zhenyao Mo wrote: > > lgtm from my point of view > > > > but please wait for ccameron to take a look > > not lgtm because I don't see how this would not regress crbug.com/518744 Agree -- we can't allow glTexImage2D calls on Mac because we can't mix glTexImage2D calls and CGLTexImageIOSurface2D calls on the same texture (yay undocumented behavior!).
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/patch-status/1547873002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547873002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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/patch-status/1547873002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547873002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
piman: PTAL https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (left): https://codereview.chromium.org/1547873002/diff/210001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1939: } On 2016/01/05 02:13:23, piman (OOO until 2016-1-4) wrote: > On 2016/01/05 01:45:15, erikchen wrote: > > On 2016/01/05 01:36:39, piman (OOO until 2016-1-4) wrote: > > > Your change seems unrelated to glTexImage2D and friends, where we still need > > to > > > disable GL_TEXTURE_RECTANGLE_ARB as a target (IIUC because uploading > software > > > pixels to an HW-backed IOSurface causes trouble), so I think you need to > keep > > > this here. > > > > I commented on this in patch set 10: > > > https://codereview.chromium.org/1547873002/diff/100002/gpu/command_buffer/ser... > > > > Could you please clarify what code will call glTexImage2D() with > > GL_TEXTURE_RECTANGLE_ARB as a target on an IOSurface backed texture? > > Actually, my recollection was slightly incorrect, the bad case is binding an > IOSurface after using glTexImage2D. I think Flash would do that - see > https://code.google.com/p/chromium/issues/detail?id=518744 - though it may have > been updated since then. The decision was to disallow uploading texels > altogether on a GL_TEXTURE_RECTANGLE_ARB texture, hence this code. > But either way, the client is untrusted, we can't assume it will only do the > right thing. > > > > I removed this check because the unit test I added needed to call > glTexImage2D() > > with target=GL_TEXTURE_RECTANGLE_ARB on a non-IOSurface backed texture. If we > > need to keep this check, can you suggest how should I restructure the unit > test? > > My suggestion would be to model the test after the use case we have for > GL_TEXTURE_RECTANGLE_ARB, namely binding an IOSurface, instead of uploading > texels. I updated the test and added this logic back to texture_manager.cc.
I don't know whether this still has the problem of potentially breaking the association between IOSurfaces and OpenGL textures, but LGTM overall. Nice work. https://codereview.chromium.org/1547873002/diff/270001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right): https://codereview.chromium.org/1547873002/diff/270001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:87: been bound as GL_TEXTURE_2D or GL_TEXTURE_RECTANGLE_ARB object. object -> objects https://codereview.chromium.org/1547873002/diff/270001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/1547873002/diff/270001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:790: glDeleteFramebuffers(1, &framebuffer_id_); Please move the Delete calls to the end of the loop if at all possible.
https://codereview.chromium.org/1547873002/diff/270001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right): https://codereview.chromium.org/1547873002/diff/270001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:87: been bound as GL_TEXTURE_2D or GL_TEXTURE_RECTANGLE_ARB object. On 2016/01/06 02:14:28, Ken Russell wrote: > object -> objects Done. https://codereview.chromium.org/1547873002/diff/270001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/1547873002/diff/270001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:790: glDeleteFramebuffers(1, &framebuffer_id_); On 2016/01/06 02:14:28, Ken Russell wrote: > Please move the Delete calls to the end of the loop if at all possible. SetUp() sets up textures with the wrong parameters for this test, and TearDown() expects to successfully delete textures/framebuffers, so this is the right place for the delete/create calls. I thought it would be simpler, and less disruptive to the other tests to maintain these assumptions than to change SetUp()/TearDown() assumptions. If you prefer, I can make those changes.
https://codereview.chromium.org/1547873002/diff/270001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/1547873002/diff/270001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:790: glDeleteFramebuffers(1, &framebuffer_id_); On 2016/01/06 02:23:37, erikchen wrote: > On 2016/01/06 02:14:28, Ken Russell wrote: > > Please move the Delete calls to the end of the loop if at all possible. > > SetUp() sets up textures with the wrong parameters for this test, and TearDown() > expects to successfully delete textures/framebuffers, so this is the right place > for the delete/create calls. > > I thought it would be simpler, and less disruptive to the other tests to > maintain these assumptions than to change SetUp()/TearDown() assumptions. If you > prefer, I can make those changes. That's fine - could you please add your first paragraph above as a comment above the glDeleteTextures call here? Thanks.
lgtm, thanks!
https://codereview.chromium.org/1547873002/diff/270001/gpu/command_buffer/tes... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/1547873002/diff/270001/gpu/command_buffer/tes... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:790: glDeleteFramebuffers(1, &framebuffer_id_); On 2016/01/06 03:46:28, Ken Russell wrote: > On 2016/01/06 02:23:37, erikchen wrote: > > On 2016/01/06 02:14:28, Ken Russell wrote: > > > Please move the Delete calls to the end of the loop if at all possible. > > > > SetUp() sets up textures with the wrong parameters for this test, and > TearDown() > > expects to successfully delete textures/framebuffers, so this is the right > place > > for the delete/create calls. > > > > I thought it would be simpler, and less disruptive to the other tests to > > maintain these assumptions than to change SetUp()/TearDown() assumptions. If > you > > prefer, I can make those changes. > > That's fine - could you please add your first paragraph above as a comment above > the glDeleteTextures call here? Thanks. Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org, kbr@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1547873002/#ps310001 (title: "Comments from kbr.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1547873002/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547873002/310001
Message was sent while issue was closed.
Description was changed from ========== Allow GL_TEXTURE_RECTANGLE_ARB as the destination for copyTextureCHROMIUM. I rewrote the vertex shader to support the new destination target. In the process, I recalculated the transforms for the vertex shader, and fixed mistakes in the original shader (it functioned incorrectly when copying sub-textures). BUG=533617 ========== to ========== Allow GL_TEXTURE_RECTANGLE_ARB as the destination for copyTextureCHROMIUM. I rewrote the vertex shader to support the new destination target. In the process, I recalculated the transforms for the vertex shader, and fixed mistakes in the original shader (it functioned incorrectly when copying sub-textures). BUG=533617 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001)
Message was sent while issue was closed.
Description was changed from ========== Allow GL_TEXTURE_RECTANGLE_ARB as the destination for copyTextureCHROMIUM. I rewrote the vertex shader to support the new destination target. In the process, I recalculated the transforms for the vertex shader, and fixed mistakes in the original shader (it functioned incorrectly when copying sub-textures). BUG=533617 ========== to ========== Allow GL_TEXTURE_RECTANGLE_ARB as the destination for copyTextureCHROMIUM. I rewrote the vertex shader to support the new destination target. In the process, I recalculated the transforms for the vertex shader, and fixed mistakes in the original shader (it functioned incorrectly when copying sub-textures). BUG=533617 Committed: https://crrev.com/6b91a5080f1ca2a3c95b1ee9308f6884b33dcd13 Cr-Commit-Position: refs/heads/master@{#367920} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/6b91a5080f1ca2a3c95b1ee9308f6884b33dcd13 Cr-Commit-Position: refs/heads/master@{#367920} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
