Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
374193002:
gpu: Optimize and cleanup code used for CHROMIUM_copy_texture. (Closed)
|
Created:
6 years, 5 months ago by dshwang Modified:
6 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptiongpu: Optimize and cleanup code used for CHROMIUM_copy_texture.
When source target and destination target is GL_TEXTURE_2D, we can use
glCopyTexImage2D() directly. It avoids gl state changes, program binding and
drawing call.
|dest_target| of DoCopyTexture() is always GL_TEXTURE_2D, so remove the
redundant argument.
Perf data are as follows:
Linux (Intel IvyBridge i7-3520M): 16 us vs 6 us -> 260% faster
Android (Nexus 5): 331.8 us vs 252.7 us -> 31% faster
BUG=N/A
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289160
Patch Set 1 #
Total comments: 6
Patch Set 2 : DoCopyTexture() branchs into fast path #
Total comments: 12
Patch Set 3 : Address reviewers commnets #
Total comments: 9
Patch Set 4 : Improve comments and DCHECK #
Total comments: 6
Patch Set 5 : Address nits #
Total comments: 2
Patch Set 6 : Fix window's webgl conformance test #
Total comments: 9
Patch Set 7 : Simplify format selecting logic and support GL_BGRA_EXT #
Total comments: 5
Patch Set 8 : remove redundant function and dcheck and tab #
Messages
Total messages: 45 (0 generated)
could you review?
I'm not fully sure. Is it maybe better left to the client? There is something nice about knowing that CopyTextureCHROMIUM is a blit and uses the 3D engines. Esp. on mobile we saw some perf issues with CopyTexImage2D, while obviously on certain architectures it might be better to use CopyTexImage2D (when the copy can be done in hw without using the 3d engine). However, hiding this as an implementation detail and using one vs. the other depending on the texture
On 2014/07/08 22:59:13, sievers wrote: > I'm not fully sure. Is it maybe better left to the client? > > There is something nice about knowing that CopyTextureCHROMIUM is a blit and > uses the 3D engines. Esp. on mobile we saw some perf issues with CopyTexImage2D, > while obviously on certain architectures it might be better to use > CopyTexImage2D (when the copy can be done in hw without using the 3d engine). > However, hiding this as an implementation detail and using one vs. the other > depending on the texture format makes it harder to see what's going on.
On 2014/07/08 23:00:04, sievers wrote: > On 2014/07/08 22:59:13, sievers wrote: > > I'm not fully sure. Is it maybe better left to the client? > > > > There is something nice about knowing that CopyTextureCHROMIUM is a blit and > > uses the 3D engines. Esp. on mobile we saw some perf issues with > CopyTexImage2D, > > while obviously on certain architectures it might be better to use > > CopyTexImage2D (when the copy can be done in hw without using the 3d engine). > > However, hiding this as an implementation detail and using one vs. the other > > depending on the texture format makes it harder to see what's going on. Thank you for your opinion. I also considered client use this technique via if statement. There is one big obstacle; FBO. The client need to manage additional FBO or create temporary FBO every time. It make client code bloat. Additional obstacles are that client code is hard to know what is restore state, especially FBO. e.g. WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(), WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture() In addition, most cases of Video-WebGL copy and Video-Canvas copy go to this fast path.
On 2014/07/09 06:45:49, dshwang wrote: > On 2014/07/08 23:00:04, sievers wrote: > > On 2014/07/08 22:59:13, sievers wrote: > > > I'm not fully sure. Is it maybe better left to the client? > > > > > > There is something nice about knowing that CopyTextureCHROMIUM is a blit and > > > uses the 3D engines. Esp. on mobile we saw some perf issues with > > CopyTexImage2D, > > > while obviously on certain architectures it might be better to use > > > CopyTexImage2D (when the copy can be done in hw without using the 3d > engine). > > > However, hiding this as an implementation detail and using one vs. the other > > > depending on the texture format makes it harder to see what's going on. > > Thank you for your opinion. I also considered client use this technique via if > statement. > There is one big obstacle; FBO. The client need to manage additional FBO or > create temporary FBO every time. > It make client code bloat. > Additional obstacles are that client code is hard to know what is restore state, > especially FBO. > e.g. WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(), > WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture() > +reveman Ok sounds good then. We can then decide later if we need some device-/driver-specific flag on the service-side to prefer blit-over-CopyTexImage2D. It's probably actually better to make this decision in a single place. > In addition, most cases of Video-WebGL copy and Video-Canvas copy go to this > fast path. Note that there are some call sites in blink which copy from an existing FBO, and ironically the original motivation there to switch from CopyTexImage2D to CopyTextureCHROMIUM was that back then mailboxes didn't support sharing textures when they were bound to an FBO. But this is not a limitation anymore.
https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:278: GLsizei height) { Does this need to be limited to source_target=TEXTURE_2D? Seems like TEXTURE_REXTANGLE_ARB should work fine too. https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:355: fragment_shader_id == FRAGMENT_SHADER_COPY_TEXTURE_2D && I think it's more appropriate to check the parameters. ie. source_target, premultiply_alpha.. Also, please move this optimization to ::DoCopyTexture instead and require the client to use that function instead of the "WithTransform" version. It's silly to pass kDefaultMatrix to this function and then check if it's equal to kDefaultMatrix..
Thank you for review! https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:278: GLsizei height) { Do you mean TEXTURE_REXTANGLE_ARB texture can be bound in FBO? as follow; glBindTexture(TEXTURE_REXTANGLE_ARB, texture_id); // NVidia drivers require texture settings to be a certain way // or they won't report FRAMEBUFFER_COMPLETE. glTexParameterf(TEXTURE_REXTANGLE_ARB, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); glTexParameterf(TEXTURE_REXTANGLE_ARB, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); glTexParameteri(TEXTURE_REXTANGLE_ARB, GL_TEXTURE_MAG_FILTER, GL_NEAREST); glTexParameteri(TEXTURE_REXTANGLE_ARB, GL_TEXTURE_MIN_FILTER, GL_NEAREST); glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, framebuffer_); glFramebufferTexture2DEXT( GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, TEXTURE_REXTANGLE_ARB, texture_id, level); I did because glFramebufferTexture2D doc doesn't mention about GL_TEXTURE_REXTANGLE_ARB ; https://www.khronos.org/opengles/sdk/docs/man/xhtml/glFramebufferTexture2D.xml https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:355: fragment_shader_id == FRAGMENT_SHADER_COPY_TEXTURE_2D && Alright. I'll update.
https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:278: GLsizei height) { On 2014/07/09 18:53:12, dshwang wrote: > Do you mean TEXTURE_REXTANGLE_ARB texture can be bound in FBO? as follow; > > glBindTexture(TEXTURE_REXTANGLE_ARB, texture_id); > // NVidia drivers require texture settings to be a certain way > // or they won't report FRAMEBUFFER_COMPLETE. > glTexParameterf(TEXTURE_REXTANGLE_ARB, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); > glTexParameterf(TEXTURE_REXTANGLE_ARB, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); > glTexParameteri(TEXTURE_REXTANGLE_ARB, GL_TEXTURE_MAG_FILTER, GL_NEAREST); > glTexParameteri(TEXTURE_REXTANGLE_ARB, GL_TEXTURE_MIN_FILTER, GL_NEAREST); > glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, framebuffer_); > glFramebufferTexture2DEXT( > GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, TEXTURE_REXTANGLE_ARB, texture_id, > level); > > I did because glFramebufferTexture2D doc doesn't mention about > GL_TEXTURE_REXTANGLE_ARB ; > https://www.khronos.org/opengles/sdk/docs/man/xhtml/glFramebufferTexture2D.xml Hm, maybe this is only supported by standard GL. The FBO spec explicitly lists TEXTURE_RECTANGLE_ARB as a valid target: http://www.opengl.org/registry/specs/EXT/framebuffer_object.txt It's fine with me to restrict this to TEXTURE_2D in the conditional that determines if this optimization should be used or not for now but I prefer if ::BindFramebufferTexture2D and ::DoFastCopyTexture are not limited to this in any way. https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (left): https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:293: } do you need to move this code? https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:302: } do you need to move this code? https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:262: (premultiply_alpha && unpremultiply_alpha); I think it would be cleaner and easier to read if you made this: bool premultiply_alpha_change = premultiply_alpha ^ unpremultiply_alpha; https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:270: height); are you missing a "return" statement here? https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:59: GLsizei height); The name "DoFastCopyTexture" assumes that this is always faster. Can we use a better name that doesn't make that assumption? We probably have to add a setting to determine if this is faster or not and it would be awkward if the name of that setting would need to be "fast_copy_texture_is_faster" to match current naming... Also, does this need to be a member function? https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:60: bool BindFramebufferTexture2D(GLuint texture_id, GLint level); Does this need to be a member function? I'd prefer if you moved this to an anonymous namespace in the .cc file if possible.
Thank you for deep review. I addressed your comments. I measured performance on both desktop and Android. Both cases show significant performance increasing. Linux (Intel IvyBridge i7-3520M): 16 us vs 6 us -> 260% faster Android (Nexus 5): 331.8 us vs 252.7 us -> 31% faster The low data and additional code changes to measure is in https://gist.github.com/ds-hwang/26e765892240aeb6e56b As increasing perf on both desktop and mobile, I turn on it by default. If you think we need additional switch, I'll define new switch and disable it by default. WDYT? https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/1/gpu/command_buffer/service/g... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:278: GLsizei height) { Yep, I restrict this to TEXTURE_2D, but make ::BindFramebufferTexture2D and ::DoFastCopyTexture be allowed to this. https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (left): https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:293: } Not really, but it's good to move, because we can save redundant searching hashmap. I mix clean-up and optimization in this CL. If you want to separate, I'll gladly do that. https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:302: } Ditto. https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:262: (premultiply_alpha && unpremultiply_alpha); That's cool. Done. https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:270: height); Oops, sorry. Done. https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:60: bool BindFramebufferTexture2D(GLuint texture_id, GLint level); Done. Only one reason is framebuffer_ member, but I moved it to anonymous namespace.
https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (left): https://codereview.chromium.org/374193002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:293: } On 2014/07/10 15:19:46, dshwang wrote: > Not really, but it's good to move, because we can save redundant searching > hashmap. > > I mix clean-up and optimization in this CL. If you want to separate, I'll gladly > do that. That's fine. I just wanted to know if there was something to this that I missed. https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:265: DCHECK(!buffer_id_ && !framebuffer_); DCHECK(!buffer_id_); DCHECK(!framebuffer_); is better as it will show exactly what caused a failure. https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:273: DCHECK(!buffer_id_ && !framebuffer_ && programs_.empty()); split DCHECK into multiple lines here too. https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:23: // via a blit to a framebuffer object. Please update this comment. Blit to FBO is no longer the only way this copy operation is performed. I think it would also be nice to mention that source_target is assumed to be TEXTURE_2D now that you removed that parameter from the copy functions. https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:36: bool unpremultiply_alpha); can you "cl format" this file to clean up these parameters while here? it might also be good to add a comment above this function to explain what |internalformat| is.
Thank you for review again. https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:265: DCHECK(!buffer_id_ && !framebuffer_); Done. https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:273: DCHECK(!buffer_id_ && !framebuffer_ && programs_.empty()); Done. https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:23: // via a blit to a framebuffer object. yes, Done. https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:36: bool unpremultiply_alpha); I wanted to run "cl format" but it changes many lines, which might make reviewer confused. Now I gladly run "cl format". :)
lgtm with nit https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/374193002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:36: bool unpremultiply_alpha); On 2014/07/10 19:45:34, dshwang wrote: > I wanted to run "cl format" but it changes many lines, which might make reviewer > confused. Now I gladly run "cl format". :) Yes, I think that was a good call. Thanks. https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:48: // e.g. GL_RGBA, GL_RGB nit: this applies to DoCopyTexture as well. maybe move the comment above to DoCopyTexture and say something like "Same as DoCopyTexture but will apply a transform..." in this comment instead. alternatively, you can probably add dest_ prefix to level/internal_format and just keep the comments as before this patch. just make sure you add dest_ prefix to the .cc file too in that case.
lgtm https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:326: bool premultiply_alpha_change = premultiply_alpha ^ unpremultiply_alpha; != for booleans
https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:326: bool premultiply_alpha_change = premultiply_alpha ^ unpremultiply_alpha; On 2014/07/10 21:04:32, sievers wrote: > != for booleans Could you explain more? As I understand, you want to change as follow, right? bool premultiply_alpha_change = !!(premultiply_alpha ^ unpremultiply_alpha);
Thank you for review. Could you check one more time, because I changed this CL little more. https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:48: // e.g. GL_RGBA, GL_RGB Thank you for detail suggestions. I chose the second option; dest_ prefix https://codereview.chromium.org/374193002/diff/80001/content/common/gpu/media... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/374193002/diff/80001/content/common/gpu/media... content/common/gpu/media/android_video_decode_accelerator.cc:366: copier_->DoCopyTextureWithTransform(gl_decoder_.get(), I changed here like gles2_cmd_decoder. Could you review here again? hkuang@, I added TODO same to comment in gles2_cmd_decoder. https://codereview.chromium.org/374193002/diff/80001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right): https://codereview.chromium.org/374193002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:51: GLint dest_level, DoCopyTextureWithTransform don't need |dest_internal_format|.
still lgtm
https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:326: bool premultiply_alpha_change = premultiply_alpha ^ unpremultiply_alpha; On 2014/07/10 22:00:19, dshwang wrote: > On 2014/07/10 21:04:32, sievers wrote: > > != for booleans > > Could you explain more? > As I understand, you want to change as follow, right? > bool premultiply_alpha_change = !!(premultiply_alpha ^ unpremultiply_alpha); Sorry I meant premultiply_alpha || unpremultiply_alpha (boolean operators instead of bitwise). And should we just override both to false here if both are true for the nonsensical case? Then you could also simplify GetFragmentShaderId() slightly.
https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:326: bool premultiply_alpha_change = premultiply_alpha ^ unpremultiply_alpha; On 2014/07/11 00:02:23, sievers wrote: > On 2014/07/10 22:00:19, dshwang wrote: > > On 2014/07/10 21:04:32, sievers wrote: > > > != for booleans > > > > Could you explain more? > > As I understand, you want to change as follow, right? > > bool premultiply_alpha_change = !!(premultiply_alpha ^ unpremultiply_alpha); > > Sorry I meant premultiply_alpha || unpremultiply_alpha (boolean operators > instead of bitwise). And should we just override both to false here if both are > true for the nonsensical case? Then you could also simplify > GetFragmentShaderId() slightly. Initial patch used boolean operators and I suggested using xor instead as I think that's easier to read. I don't have a strong preference as long as we keep it consistent with GetFragmentShaderId. Please make it an enum if we're not going to allow both to be set. However, I think that's better left to a follow up patch.
On 2014/07/11 00:34:54, reveman wrote: > https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... > File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): > > https://codereview.chromium.org/374193002/diff/60001/gpu/command_buffer/servi... > gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:326: bool > premultiply_alpha_change = premultiply_alpha ^ unpremultiply_alpha; > On 2014/07/11 00:02:23, sievers wrote: > > On 2014/07/10 22:00:19, dshwang wrote: > > > On 2014/07/10 21:04:32, sievers wrote: > > > > != for booleans > > > > > > Could you explain more? > > > As I understand, you want to change as follow, right? > > > bool premultiply_alpha_change = !!(premultiply_alpha ^ unpremultiply_alpha); > > > > Sorry I meant premultiply_alpha || unpremultiply_alpha (boolean operators > > instead of bitwise). And should we just override both to false here if both > are > > true for the nonsensical case? Then you could also simplify > > GetFragmentShaderId() slightly. > > Initial patch used boolean operators and I suggested using xor instead as I > think that's easier to read. I don't have a strong preference as long as we keep > it consistent with GetFragmentShaderId. > > Please make it an enum if we're not going to allow both to be set. However, I > think that's better left to a follow up patch. Ok, or let's just leave as is then.
Thank you all for great review!
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/374193002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/374193002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
On 2014/07/11 15:54:28, I haz the power (commit-bot) wrote: > FYI, CQ is re-trying this CL (attempt #1). > The failing builders are: > win_gpu_triggered_tests on tryserver.chromium.gpu > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...) conformance_context_context_attribute_preserve_drawing_buffer on win gpu bot might not be flaky, but I cannot reproduce it on linux. Let me investigate.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Hi, sorry for delay to re-upload. It took time to get windows machine. Fortunately, win gpu bot catched bug of previous patch set. I fixed it. I commented the new changes. reveman@, Could you review again? https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right): https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:61: GL_RGBA (R, G, B, A) It's actually the rule of texture2d() built-in functions of glsl. So we don't need to do anything. I just clarify what's happen. https://codereview.chromium.org/374193002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:357: // so restrict this to GL_TEXTURE_2D. This code resolves win gpu bot failure in context/context-attribute-preserve-drawing-buffer.html The test draws RGB webgl on RGBA canvas 2d, so |source_internal_format| is RGB and |dest_internal_format| is RGBA. Current implementation can handle it thank to texture2d() in glsl. but glCopyTexImage2D() reports GL_INVALID_OPERATION error. ANGLE strictly checks format while real OpenGL drivers naively check. It's why only Win gpu bot fails with previous patch set.
https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right): https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:50: specified by <internal_format>. Must be one of the following symbolic constants: Please format this comment so each line is 80 characters or less. https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:53: fill the components by following rules although glCopyImage2D generates GL_INVALID_OPERATION in this case. what is glCopyImage2D? and why is it relevant to mention it as part of this spec? does the user of this extension need to know this? https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:61: GL_RGBA (R, G, B, A) On 2014/08/11 14:08:12, dshwang wrote: > It's actually the rule of texture2d() built-in functions of glsl. > So we don't need to do anything. I just clarify what's happen. Acknowledged. https://codereview.chromium.org/374193002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:315: GLenum dest_format) { is this worth it or should we just keep it simple and only use CopyTexImage2D if format is exactly the same?
Thank you for quick review. win and mac bot failed because source texture format can be GL_BGRA_EXT. New patch set allows GL_BGRA_EXT as source texture format. However still don't allow GL_BGRA_EXT as target texture format because there is not any code using GL_BGRA_EXT as target texture format and I'm not sure current glCopyTextureChromium implementation can handle it. https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right): https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:50: specified by <internal_format>. Must be one of the following symbolic constants: On 2014/08/11 18:37:21, reveman wrote: > Please format this comment so each line is 80 characters or less. Done. https://codereview.chromium.org/374193002/diff/100001/gpu/GLES2/extensions/CH... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:53: fill the components by following rules although glCopyImage2D generates GL_INVALID_OPERATION in this case. On 2014/08/11 18:37:21, reveman wrote: > what is glCopyImage2D? and why is it relevant to mention it as part of this > spec? does the user of this extension need to know this? Done. Remove it. https://codereview.chromium.org/374193002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:315: GLenum dest_format) { On 2014/08/11 18:37:21, reveman wrote: > is this worth it or should we just keep it simple and only use CopyTexImage2D if > format is exactly the same? Simplified as follows; return source_format == dest_format || (source_format == GL_RGBA && dest_format == GL_RGB); IMO GL_RGBA->GL_RGB needs to be optimized because it often happens. e.g. webgl->webgl copy, video->webgl copy, video->canvas copy.
lgtm with a nit and some optional adjustments https://codereview.chromium.org/374193002/diff/120001/gpu/GLES2/extensions/CH... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right): https://codereview.chromium.org/374193002/diff/120001/gpu/GLES2/extensions/CH... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:61: GL_RGBA (R, G, B, A) nit: please use spaces instead of tabs. https://codereview.chromium.org/374193002/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:252: GLenum dest_format) { Is it really worth having this function? If you want to keep it then maybe add "format" to the name somehow so it's clear what we mean when we say superset of destination. ie. IsSourceFormatSupersetOfDestinationFormat https://codereview.chromium.org/374193002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:257: source_format == GL_BGRA_EXT); Do you need these DCHECKs here? It doesn't seem relevant when looking at what the function does.
On 2014/08/11 20:48:46, reveman wrote: > lgtm with a nit and some optional adjustments Thank you for quick and good review! https://codereview.chromium.org/374193002/diff/120001/gpu/GLES2/extensions/CH... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right): https://codereview.chromium.org/374193002/diff/120001/gpu/GLES2/extensions/CH... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:61: GL_RGBA (R, G, B, A) On 2014/08/11 20:48:46, reveman wrote: > nit: please use spaces instead of tabs. Done. https://codereview.chromium.org/374193002/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right): https://codereview.chromium.org/374193002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:252: GLenum dest_format) { On 2014/08/11 20:48:46, reveman wrote: > Is it really worth having this function? If you want to keep it then maybe add > "format" to the name somehow so it's clear what we mean when we say superset of > destination. ie. IsSourceFormatSupersetOfDestinationFormat I remove this function and reuse your naming for local variable.
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/374193002/14...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/374193002/14...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Message was sent while issue was closed.
Change committed as 289160 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
