Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
118203002:
During virtual context switches only restore texture units that have changed from the previous cont… (Closed)
|
Created:
7 years ago by kaanb Modified:
6 years, 11 months ago CC:
chromium-reviews, piman+watch_chromium.org, Tom Hudson, boliu, klobag.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDuring virtual context switches only restore texture units
that have changed from the previous context.
The most CPU consuming operations on the GPU thread for
Android are virtual context switches. We attempt to reduce the CPU usage by
context switches with this patch.
BUG=244701
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245109
Patch Set 1 #Patch Set 2 : Cleanup #Patch Set 3 : Remove TODO #Patch Set 4 : Add a comment #
Total comments: 7
Patch Set 5 : Use the delta between the ContextStates to decide if we should bind a texture or not #
Total comments: 13
Patch Set 6 : Code review feedback #
Total comments: 6
Patch Set 7 : Incorporate comments and add unit tests #
Total comments: 2
Patch Set 8 : More unittests #
Total comments: 2
Patch Set 9 : Refactor unittests #
Total comments: 11
Patch Set 10 : Change GLStateRestorer API #Patch Set 11 : Incorporate sievers' feedback on unittests #
Total comments: 6
Patch Set 12 : Code review feedback, pass const GLStateRestorer* in the API, make GLStateRestorerImpl::GetContextS… #Patch Set 13 : Rebase #Patch Set 14 : s/class ContextState/struct ContextState/ #Patch Set 15 : s/class ContextState/struct ContextState/ #Patch Set 16 : Change mock GetContextState to return const ContextState* instead of non-const #Messages
Total messages: 54 (0 generated)
Thanks, this looks much cleaner than the earlier patch. It would be interesting to know if this shows with any of the canvas perf tests. https://codereview.chromium.org/118203002/diff/60001/ui/gl/gl_gl_api_implemen... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/118203002/diff/60001/ui/gl/gl_gl_api_implemen... ui/gl/gl_gl_api_implementation.cc:313: (current_context_) It might be worth checking how often we go from MakeCurrent(NULL) => MakeCurrent(some_context) since that would defeat this optimization. I'm not sure if that ever happens with virtual contexts though.
https://codereview.chromium.org/118203002/diff/60001/ui/gl/gl_gl_api_implemen... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/118203002/diff/60001/ui/gl/gl_gl_api_implemen... ui/gl/gl_gl_api_implementation.cc:313: (current_context_) On 2013/12/18 16:04:07, Sami wrote: > It might be worth checking how often we go from MakeCurrent(NULL) => > MakeCurrent(some_context) since that would defeat this optimization. I'm not > sure if that ever happens with virtual contexts though. I see MakeCurrent(NULL) -> MakeCurrent(some_context) transition a few times when I first open a tab but not afterwards.
https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:111: max_active_texture_unit_) + 1; Do we really need |max_active_texture_unit_| which seems to track which units *might* be set to something else than the default textures? But it doesn't know when we set textures back to the default texture, and it also doesn't know about which target was changed. But luckily we have all this information... :) So.... can't you just compare the old and new service id in RestoreTextureUnitBindings()? That would actually eliminate even more GL calls (in fact, all redundant ones).
https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:111: max_active_texture_unit_) + 1; On 2013/12/18 19:36:20, sievers wrote: > Do we really need |max_active_texture_unit_| which seems to track which units > *might* be set to something else than the default textures? But it doesn't know > when we set textures back to the default texture, and it also doesn't know about > which target was changed. But luckily we have all this information... :) > > So.... can't you just compare the old and new service id in > RestoreTextureUnitBindings()? That would actually eliminate even more GL calls > (in fact, all redundant ones). Having max_active_texture_unit_ is useful as it reduces the number of iterations of the loop. I was planning to land this patch first and then go into target specific optimizations in RestoreTextureUnitBindings() after the holidays. Even with this approach, we go down from 32 * 3 GL calls per switch to 1 * 3 GL calls so it is still a huge win IMHO.
On 2013/12/18 19:36:19, sievers wrote: > https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... > File gpu/command_buffer/service/context_state.cc (right): > > https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... > gpu/command_buffer/service/context_state.cc:111: max_active_texture_unit_) + 1; > Do we really need |max_active_texture_unit_| which seems to track which units > *might* be set to something else than the default textures? But it doesn't know > when we set textures back to the default texture, and it also doesn't know about > which target was changed. But luckily we have all this information... :) > > So.... can't you just compare the old and new service id in > RestoreTextureUnitBindings()? That would actually eliminate even more GL calls > (in fact, all redundant ones). Also, what does this do to the bug? How much does change the state restore time spent?
On 2013/12/18 19:44:55, sievers wrote: > On 2013/12/18 19:36:19, sievers wrote: > > > https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... > > File gpu/command_buffer/service/context_state.cc (right): > > > > > https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... > > gpu/command_buffer/service/context_state.cc:111: max_active_texture_unit_) + > 1; > > Do we really need |max_active_texture_unit_| which seems to track which units > > *might* be set to something else than the default textures? But it doesn't > know > > when we set textures back to the default texture, and it also doesn't know > about > > which target was changed. But luckily we have all this information... :) > > > > So.... can't you just compare the old and new service id in > > RestoreTextureUnitBindings()? That would actually eliminate even more GL calls > > (in fact, all redundant ones). > > Also, what does this do to the bug? How much does change the state restore time > spent? I counted GL calls (which go down by 1/32) but didn't measure the wall time. Another benefit of landing this patch would be to see if it has any impact on the perf bots. I tried the script used by the perf bot locally but unfortunately it kept timing outs.
https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:107: size_t units_to_restore = texture_units.size(); Should this be max_active_texture_unit_? https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:111: max_active_texture_unit_) + 1; On 2013/12/18 19:43:56, kaanb wrote: > On 2013/12/18 19:36:20, sievers wrote: > > Do we really need |max_active_texture_unit_| which seems to track which units > > *might* be set to something else than the default textures? But it doesn't > know > > when we set textures back to the default texture, and it also doesn't know > about > > which target was changed. But luckily we have all this information... :) > > > > So.... can't you just compare the old and new service id in > > RestoreTextureUnitBindings()? That would actually eliminate even more GL calls > > (in fact, all redundant ones). > > Having max_active_texture_unit_ is useful as it reduces the number of iterations > of the loop. I was planning to land this patch first and then go into target > specific optimizations in RestoreTextureUnitBindings() after the holidays. > > Even with this approach, we go down from 32 * 3 GL calls per switch to 1 * 3 GL > calls so it is still a huge win IMHO. Just diffing against prev_state would be great for android webview. If we could plug in the app's state as prev_state, then we could save a lot of GL calls when switching between app and chromium.
On 2013/12/18 19:43:56, kaanb wrote: > https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... > File gpu/command_buffer/service/context_state.cc (right): > > https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... > gpu/command_buffer/service/context_state.cc:111: max_active_texture_unit_) + 1; > On 2013/12/18 19:36:20, sievers wrote: > > Do we really need |max_active_texture_unit_| which seems to track which units > > *might* be set to something else than the default textures? But it doesn't > know > > when we set textures back to the default texture, and it also doesn't know > about > > which target was changed. But luckily we have all this information... :) > > > > So.... can't you just compare the old and new service id in > > RestoreTextureUnitBindings()? That would actually eliminate even more GL calls > > (in fact, all redundant ones). > > Having max_active_texture_unit_ is useful as it reduces the number of iterations > of the loop. I don't think that is a real-world optimization (worth the cost of adding state tracking, making the code more complicated and potentially introducing bugs). We don't understand yet where the cost (0.5ms according to the bug) comes from and if it can even be avoided, but it's likely spent in the driver. It's also possible that it comes from real changes rather than redundant calls for textures units and targets (that are currently set to 0 and that we redundantly set to 0), in which case none of this will help. That's why I was curious about the measurement. I was planning to land this patch first and then go into target > specific optimizations in RestoreTextureUnitBindings() after the holidays. > I think using the existing state we have and avoiding GL calls is a more straightforward direction. We could do the same for the global state later. > Even with this approach, we go down from 32 * 3 GL calls per switch to 1 * 3 GL > calls so it is still a huge win IMHO. Did you measure it?
Please add a unit test to gles2_cmd_decoder_unittest.cc. https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:111: max_active_texture_unit_) + 1; On 2013/12/18 19:43:56, kaanb wrote: > On 2013/12/18 19:36:20, sievers wrote: > > Do we really need |max_active_texture_unit_| which seems to track which units > > *might* be set to something else than the default textures? But it doesn't > know > > when we set textures back to the default texture, and it also doesn't know > about > > which target was changed. But luckily we have all this information... :) > > > > So.... can't you just compare the old and new service id in > > RestoreTextureUnitBindings()? That would actually eliminate even more GL calls > > (in fact, all redundant ones). > > Having max_active_texture_unit_ is useful as it reduces the number of iterations > of the loop. I was planning to land this patch first and then go into target > specific optimizations in RestoreTextureUnitBindings() after the holidays. Reducing the number of loop iterations is a premature optimization that is known to not be fully correct. Please remove the newly added max_active_texture_unit_ state and just iterate over all texture units. If it's later determined that the optimization would help, it can be added.
Unit tests are still work-in-progress but the rest of the patch is ready for another round of reviews, PTAL
https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:78: Can you just put 'if (prev_state)' around the whole block here? https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:80: (prev_state) ? &prev_state->texture_units[unit] : NULL; nit: no need for '(',')' https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:90: bool binding_2d_changed = prev_service_id_2d != service_id_2d; There is one bug here: - context A binds texture 0 - switch to context B - bind texture N (>0) - delete context B - come back to context A and we don't have prev_state: prev_service_id will be 0 and we won't restore the binding. I will still have N bound. You could just rename these to 'bind_texture_2d' etc. and - initialize them to |true| - only set them inside the 'if (prev_state)'-block otherwise - for external/rectangle put the feature flag check from below into that condition That should fix it and make it slightly more readable.
I think there's another problem: In GLES2DecoderImpl::Initialize() we set all texture units (up to max.) to the 'default texture', which is actually a real texture that we create for each target. These are not shared between browser and render compositor, so the optimization in this patch might not do anything as long as that is the case.
https://codereview.chromium.org/118203002/diff/80001/ui/gl/gl_gl_api_implemen... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/118203002/diff/80001/ui/gl/gl_gl_api_implemen... ui/gl/gl_gl_api_implementation.cc:316: (current_context_) && !switched_contexts I.e. only if we are using the same underlying real GL context.
https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:90: bool binding_2d_changed = prev_service_id_2d != service_id_2d; On 2013/12/19 19:35:38, sievers wrote: > There is one bug here: > - context A binds texture 0 > - switch to context B > - bind texture N (>0) > - delete context B > - come back to context A and we don't have prev_state: > prev_service_id will be 0 and we won't restore the binding. I will still have N > bound. Doesn't the code below already handle this case? If there's no previous state then it unconditionally re-binds the textures to all of the binding points. https://codereview.chromium.org/118203002/diff/80001/ui/gl/gl_gl_api_implemen... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/118203002/diff/80001/ui/gl/gl_gl_api_implemen... ui/gl/gl_gl_api_implementation.cc:313: LOG(ERROR) << "KAANB: besiktas!"; What does this mean? :)
https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:90: bool binding_2d_changed = prev_service_id_2d != service_id_2d; On 2013/12/19 21:39:33, Ken Russell wrote: > On 2013/12/19 19:35:38, sievers wrote: > > There is one bug here: > > - context A binds texture 0 > > - switch to context B > > - bind texture N (>0) > > - delete context B > > - come back to context A and we don't have prev_state: > > prev_service_id will be 0 and we won't restore the binding. I will still have > N > > bound. > > Doesn't the code below already handle this case? If there's no previous state > then it unconditionally re-binds the textures to all of the binding points. Actually, you are right, I missed that. But still think it could be simplified for readability by having less convoluted checks :)
https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:90: bool binding_2d_changed = prev_service_id_2d != service_id_2d; On 2013/12/19 21:44:18, sievers wrote: > On 2013/12/19 21:39:33, Ken Russell wrote: > > On 2013/12/19 19:35:38, sievers wrote: > > > There is one bug here: > > > - context A binds texture 0 > > > - switch to context B > > > - bind texture N (>0) > > > - delete context B > > > - come back to context A and we don't have prev_state: > > > prev_service_id will be 0 and we won't restore the binding. I will still > have > > N > > > bound. > > > > Doesn't the code below already handle this case? If there's no previous state > > then it unconditionally re-binds the textures to all of the binding points. > > Actually, you are right, I missed that. But still think it could be simplified > for readability by having less convoluted checks :) Yes, I agree. :) It's a lot of code for a simple side-effect, and more thought could probably reduce it dramatically.
Unittests need more work but the rest is ready for another round https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:78: On 2013/12/19 19:35:38, sievers wrote: > Can you just put 'if (prev_state)' around the whole block here? Done. https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:80: (prev_state) ? &prev_state->texture_units[unit] : NULL; On 2013/12/19 19:35:38, sievers wrote: > nit: no need for '(',')' I removed all ternary conditional operators. https://codereview.chromium.org/118203002/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/context_state.cc:90: bool binding_2d_changed = prev_service_id_2d != service_id_2d; On 2013/12/19 22:13:16, Ken Russell wrote: > On 2013/12/19 21:44:18, sievers wrote: > > On 2013/12/19 21:39:33, Ken Russell wrote: > > > On 2013/12/19 19:35:38, sievers wrote: > > > > There is one bug here: > > > > - context A binds texture 0 > > > > - switch to context B > > > > - bind texture N (>0) > > > > - delete context B > > > > - come back to context A and we don't have prev_state: > > > > prev_service_id will be 0 and we won't restore the binding. I will still > > have > > > N > > > > bound. > > > > > > Doesn't the code below already handle this case? If there's no previous > state > > > then it unconditionally re-binds the textures to all of the binding points. > > > > Actually, you are right, I missed that. But still think it could be simplified > > for readability by having less convoluted checks :) > > Yes, I agree. :) It's a lot of code for a simple side-effect, and more thought > could probably reduce it dramatically. I simplified the code as suggested by sievers@, I'm adding a unittest for the case he mentioned anyway and hopefully it is easier to see now that we bind all targets when prev_state is NULL. https://codereview.chromium.org/118203002/diff/80001/ui/gl/gl_gl_api_implemen... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/118203002/diff/80001/ui/gl/gl_gl_api_implemen... ui/gl/gl_gl_api_implementation.cc:313: LOG(ERROR) << "KAANB: besiktas!"; On 2013/12/19 21:39:33, Ken Russell wrote: > What does this mean? :) It's the name of the soccer team I support :) I needed a unique string in the logs for debugging https://codereview.chromium.org/118203002/diff/80001/ui/gl/gl_gl_api_implemen... ui/gl/gl_gl_api_implementation.cc:316: (current_context_) On 2013/12/19 21:13:50, sievers wrote: > && !switched_contexts > > I.e. only if we are using the same underlying real GL context. Done.
Thanks, looks good. If you get a chance you might want to try if the default textures really keep this patch from having any effect (between browser and render compositor). Because then we'll also have to come up with something to address that. https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/context_state.cc:85: const TextureUnit* prev_unit = &prev_state->texture_units[unit]; nit: now you can make prev_unit a const-reference instead https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8833: SetupTexture(); You could actually put the expectations in the right order and put InSequence sequence; https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8834: // TODO(kaanb): do not hardcode size I think group().max_texture_units() is what you are looking for.
On 2013/12/20 17:37:06, sievers wrote: > Thanks, looks good. > > If you get a chance you might want to try if the default textures really keep > this patch from having any effect (between browser and render compositor). > Because then we'll also have to come up with something to address that. > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > File gpu/command_buffer/service/context_state.cc (right): > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > gpu/command_buffer/service/context_state.cc:85: const TextureUnit* prev_unit = > &prev_state->texture_units[unit]; > nit: now you can make prev_unit a const-reference instead > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8833: SetupTexture(); > You could actually put the expectations in the right order and put > InSequence sequence; > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8834: // TODO(kaanb): > do not hardcode size > I think group().max_texture_units() is what you are looking for. I see that for 1051 texture units, we hit the early-out case 341 times. Another interesting point is if we don't early out we almost always re-bind all targets. I asked boliu@ to test the impact on Android WebView.
I fixed up the unit tests and everything is ready for another look. https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/context_state.cc:85: const TextureUnit* prev_unit = &prev_state->texture_units[unit]; On 2013/12/20 17:37:07, sievers wrote: > nit: now you can make prev_unit a const-reference instead Done. https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8833: SetupTexture(); On 2013/12/20 17:37:07, sievers wrote: > You could actually put the expectations in the right order and put > InSequence sequence; Done. https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8834: // TODO(kaanb): do not hardcode size On 2013/12/20 17:37:07, sievers wrote: > I think group().max_texture_units() is what you are looking for. Done.
https://codereview.chromium.org/118203002/diff/180001/ui/gl/gl_gl_api_impleme... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/118203002/diff/180001/ui/gl/gl_gl_api_impleme... ui/gl/gl_gl_api_implementation.cc:313: (current_context_ && !switched_contexts) This patch is currently a no-op for webview because we call SetGLToRealGLApi when entering/exiting draw functor to save/restore the app gl state. Thus switched_contexts is true here and NULL is passed to RestoreState. (Note it's a bit different from I told in person) However if we could pass in the saved app state into this RestoreState, we'd be golden.
https://codereview.chromium.org/118203002/diff/180001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/118203002/diff/180001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8872: TEST_F(GLES2DecoderManualInitTest, RestoreStateWithPreviousState) { Could you please add a couple more tests? - Having a non-default texture bound to another texture unit than GL_TEXTURE0 - Switching between two ContextStates, one with a non-default texture bound to GL_TEXTURE0's GL_TEXTURE2D binding point, the other with one bound to GL_TEXTURE1, and verify the right thing happens when switching from one to the other and back
On 2014/01/08 02:56:27, Ken Russell wrote: > https://codereview.chromium.org/118203002/diff/180001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): > > https://codereview.chromium.org/118203002/diff/180001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8872: > TEST_F(GLES2DecoderManualInitTest, RestoreStateWithPreviousState) { > Could you please add a couple more tests? > > - Having a non-default texture bound to another texture unit than GL_TEXTURE0 > - Switching between two ContextStates, one with a non-default texture bound to > GL_TEXTURE0's GL_TEXTURE2D binding point, the other with one bound to > GL_TEXTURE1, and verify the right thing happens when switching from one to the > other and back Done.
https://codereview.chromium.org/118203002/diff/330001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/118203002/diff/330001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8914: GetDecoder()->RestoreAllTextureUnitBindings(&prev_state); These tests look good but there's a lot of duplicated code between them. Could you please refactor it? It's completely legal for the test function to call a helper method on the class which issues EXPECT_CALL and similar calls based on incoming arguments.
https://codereview.chromium.org/118203002/diff/330001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/118203002/diff/330001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8914: GetDecoder()->RestoreAllTextureUnitBindings(&prev_state); On 2014/01/09 00:09:08, Ken Russell wrote: > These tests look good but there's a lot of duplicated code between them. Could > you please refactor it? It's completely legal for the test function to call a > helper method on the class which issues EXPECT_CALL and similar calls based on > incoming arguments. Also, in case it's helpful, see calls to Mock::VerifyAndClearExpectations in e.g. src/cc/layers/texture_layer_unittest.cc. That might help merging RestoreStateNonDefaultUnit0 and RestoreStateNonDefaultUnit1 into a single test that just swaps back and forth between two ContextStates.
On 2014/01/07 23:32:39, kaanb wrote: > On 2013/12/20 17:37:06, sievers wrote: > > Thanks, looks good. > > > > If you get a chance you might want to try if the default textures really keep > > this patch from having any effect (between browser and render compositor). > > Because then we'll also have to come up with something to address that. > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/context_state.cc (right): > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > gpu/command_buffer/service/context_state.cc:85: const TextureUnit* prev_unit = > > &prev_state->texture_units[unit]; > > nit: now you can make prev_unit a const-reference instead > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8833: SetupTexture(); > > You could actually put the expectations in the right order and put > > InSequence sequence; > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8834: // TODO(kaanb): > > do not hardcode size > > I think group().max_texture_units() is what you are looking for. > > I see that for 1051 texture units, we hit the early-out case 341 times. Another > interesting point is if we don't early out we almost always re-bind all targets. > I asked boliu@ to test the impact on Android WebView. It could be context switches between UI thread helper context and browser compositor context that share the default textures. And this will go away with ubercomp, at which point this patch will yield no optimization (sharing between webgl and compositor is also going away). So can you file a bug to follow-up on this? We will likely have to do something about the default textures for this patch to work.
On 2014/01/09 00:09:07, Ken Russell wrote: > https://codereview.chromium.org/118203002/diff/330001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): > > https://codereview.chromium.org/118203002/diff/330001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8914: > GetDecoder()->RestoreAllTextureUnitBindings(&prev_state); > These tests look good but there's a lot of duplicated code between them. Could > you please refactor it? It's completely legal for the test function to call a > helper method on the class which issues EXPECT_CALL and similar calls based on > incoming arguments. Done.
On 2014/01/09 00:11:02, Ken Russell wrote: > https://codereview.chromium.org/118203002/diff/330001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): > > https://codereview.chromium.org/118203002/diff/330001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8914: > GetDecoder()->RestoreAllTextureUnitBindings(&prev_state); > On 2014/01/09 00:09:08, Ken Russell wrote: > > These tests look good but there's a lot of duplicated code between them. Could > > you please refactor it? It's completely legal for the test function to call a > > helper method on the class which issues EXPECT_CALL and similar calls based on > > incoming arguments. > > Also, in case it's helpful, see calls to Mock::VerifyAndClearExpectations in > e.g. src/cc/layers/texture_layer_unittest.cc. That might help merging > RestoreStateNonDefaultUnit0 and RestoreStateNonDefaultUnit1 into a single test > that just swaps back and forth between two ContextStates. Resetting the state of a unittest in the middle makes it harder to read and debug IMHO. So I'd rather keep it as two separate unittests, if you feel strongly please let me know and I'll see if I can merge them into one unittest.
On 2014/01/09 14:05:29, sievers wrote: > On 2014/01/07 23:32:39, kaanb wrote: > > On 2013/12/20 17:37:06, sievers wrote: > > > Thanks, looks good. > > > > > > If you get a chance you might want to try if the default textures really > keep > > > this patch from having any effect (between browser and render compositor). > > > Because then we'll also have to come up with something to address that. > > > > > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > > File gpu/command_buffer/service/context_state.cc (right): > > > > > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > > gpu/command_buffer/service/context_state.cc:85: const TextureUnit* prev_unit > = > > > &prev_state->texture_units[unit]; > > > nit: now you can make prev_unit a const-reference instead > > > > > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > > File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8833: > SetupTexture(); > > > You could actually put the expectations in the right order and put > > > InSequence sequence; > > > > > > > > > https://codereview.chromium.org/118203002/diff/100001/gpu/command_buffer/serv... > > > gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8834: // > TODO(kaanb): > > > do not hardcode size > > > I think group().max_texture_units() is what you are looking for. > > > > I see that for 1051 texture units, we hit the early-out case 341 times. > Another > > interesting point is if we don't early out we almost always re-bind all > targets. > > I asked boliu@ to test the impact on Android WebView. > > It could be context switches between UI thread helper context and browser > compositor context that share the default textures. > And this will go away with ubercomp, at which point this patch will yield no > optimization (sharing between webgl and compositor is also going away). > So can you file a bug to follow-up on this? We will likely have to do something > about the default textures for this patch to work. Opened crbug.com/333063
Thanks for persisting. The new patch LGTM.
+piman for gpu/ OWNERS
just a few nits. https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8901: SpecializedSetup<ActiveTexture, 0>(true); I don't see the specialization for this anywhere (and the default impl is a NOP). https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8950: InitializeContextState(&prev_state, 0, 0); nit: This would break if kServiceDefaultTexture2dId was 0. Can we explicitly set it to a valid texture id (for example kServiceTextureId)? https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8988: InitializeContextState(&prev_state, 1, 0); nit: here also use a real id (not 0). https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:98: uint32 active_unit) { This seems to be very specific to your tests. How about subclassing GLES2DecoderManualInitTest for your purposes and moving the added functions from this file to over there? Note: You want to make sure the class name starts with 'GLES2Decoder*' to satisfy existing heap check exclusion filters.
https://codereview.chromium.org/118203002/diff/400001/ui/gl/gl_state_restorer.h File ui/gl/gl_state_restorer.h (right): https://codereview.chromium.org/118203002/diff/400001/ui/gl/gl_state_restorer... ui/gl/gl_state_restorer.h:30: virtual const gpu::gles2::ContextState* GetContextState() = 0; Mmh, this causes a dependency cycle gpu -> ui -> gpu
https://codereview.chromium.org/118203002/diff/400001/ui/gl/gl_state_restorer.h File ui/gl/gl_state_restorer.h (right): https://codereview.chromium.org/118203002/diff/400001/ui/gl/gl_state_restorer... ui/gl/gl_state_restorer.h:30: virtual const gpu::gles2::ContextState* GetContextState() = 0; On 2014/01/13 22:12:46, piman wrote: > Mmh, this causes a dependency cycle gpu -> ui -> gpu It's only a forward declaration though. One alternative is using void* as the type but that violates type safety and would require static_casts. wdyt?
https://codereview.chromium.org/118203002/diff/400001/ui/gl/gl_state_restorer.h File ui/gl/gl_state_restorer.h (right): https://codereview.chromium.org/118203002/diff/400001/ui/gl/gl_state_restorer... ui/gl/gl_state_restorer.h:30: virtual const gpu::gles2::ContextState* GetContextState() = 0; On 2014/01/13 22:17:00, kaanb wrote: > On 2014/01/13 22:12:46, piman wrote: > > Mmh, this causes a dependency cycle gpu -> ui -> gpu > > It's only a forward declaration though. One alternative is using void* as the > type but that violates type safety and would require static_casts. wdyt? It would just make it worse (hiding the circular dependency, and as you mention, violating the type safety). Maybe what you want is to pass a GLStateRestorer to RestoreState, and do the GetContextState dance (using internal APIs) in the implementation in the gpu/ code.
On 2014/01/13 22:37:46, piman wrote: > https://codereview.chromium.org/118203002/diff/400001/ui/gl/gl_state_restorer.h > File ui/gl/gl_state_restorer.h (right): > > https://codereview.chromium.org/118203002/diff/400001/ui/gl/gl_state_restorer... > ui/gl/gl_state_restorer.h:30: virtual const gpu::gles2::ContextState* > GetContextState() = 0; > On 2014/01/13 22:17:00, kaanb wrote: > > On 2014/01/13 22:12:46, piman wrote: > > > Mmh, this causes a dependency cycle gpu -> ui -> gpu > > > > It's only a forward declaration though. One alternative is using void* as the > > type but that violates type safety and would require static_casts. wdyt? > > It would just make it worse (hiding the circular dependency, and as you mention, > violating the type safety). > > Maybe what you want is to pass a GLStateRestorer to RestoreState, and do the > GetContextState dance (using internal APIs) in the implementation in the gpu/ > code. I tried that but eventually you need a dynamic_cast to GLStateRestorerImpl which is disallowed. How about changing the GLStateRestorer::GetContextState to return a gfx::ContextStateBase* interface (i.e. pure virtual base class) and change gpu::gles2::ContextState to implement that interface. The interface would have methods to query texture ids bound to various targets given a texture unit id that would be called from ContextState::RestoreTextureUnitBindings.
On Mon, Jan 13, 2014 at 4:14 PM, <kaanb@chromium.org> wrote: > On 2014/01/13 22:37:46, piman wrote: > > https://codereview.chromium.org/118203002/diff/400001/ui/ > gl/gl_state_restorer.h > >> File ui/gl/gl_state_restorer.h (right): >> > > > https://codereview.chromium.org/118203002/diff/400001/ui/ > gl/gl_state_restorer.h#newcode30 > >> ui/gl/gl_state_restorer.h:30: virtual const gpu::gles2::ContextState* >> GetContextState() = 0; >> On 2014/01/13 22:17:00, kaanb wrote: >> > On 2014/01/13 22:12:46, piman wrote: >> > > Mmh, this causes a dependency cycle gpu -> ui -> gpu >> > >> > It's only a forward declaration though. One alternative is using void* >> as >> > the > >> > type but that violates type safety and would require static_casts. wdyt? >> > > It would just make it worse (hiding the circular dependency, and as you >> > mention, > >> violating the type safety). >> > > Maybe what you want is to pass a GLStateRestorer to RestoreState, and do >> the >> GetContextState dance (using internal APIs) in the implementation in the >> gpu/ >> code. >> > > I tried that but eventually you need a dynamic_cast to GLStateRestorerImpl > which > is disallowed. > Why do you need dynamic_cast? There's only one implementation of GLStateRestorer. Why couldn't you static_cast? We wouldn't mix/match different implementations anyway (I don't think it could make sense). > > How about changing the GLStateRestorer::GetContextState to return a > gfx::ContextStateBase* interface (i.e. pure virtual base class) and change > gpu::gles2::ContextState to implement that interface. The interface would > have > methods to query texture ids > bound to various targets given a texture unit id that would be called from > ContextState::RestoreTextureUnitBindings. > We could, but I don't think that's needed. > > > https://codereview.chromium.org/118203002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
When I use static_cast<> like this, the DCHECK in GetContextState fails. Is
the decoder_ field being set to NULL when we perform the static_cast<>?
void GLStateRestorerImpl::RestoreState(const gfx::GLStateRestorer*
restorer) {
DCHECK(decoder_.get());
const GLStateRestorerImpl* restorer_impl =
static_cast<const GLStateRestorerImpl*>(restorer);
decoder_->RestoreState(restorer_impl->GetContextState());
}
On Mon, Jan 13, 2014 at 4:32 PM, Antoine Labour <piman@chromium.org> wrote:
>
>
>
> On Mon, Jan 13, 2014 at 4:14 PM, <kaanb@chromium.org> wrote:
>
>> On 2014/01/13 22:37:46, piman wrote:
>>
>> https://codereview.chromium.org/118203002/diff/400001/ui/
>> gl/gl_state_restorer.h
>>
>>> File ui/gl/gl_state_restorer.h (right):
>>>
>>
>>
>> https://codereview.chromium.org/118203002/diff/400001/ui/
>> gl/gl_state_restorer.h#newcode30
>>
>>> ui/gl/gl_state_restorer.h:30: virtual const gpu::gles2::ContextState*
>>> GetContextState() = 0;
>>> On 2014/01/13 22:17:00, kaanb wrote:
>>> > On 2014/01/13 22:12:46, piman wrote:
>>> > > Mmh, this causes a dependency cycle gpu -> ui -> gpu
>>> >
>>> > It's only a forward declaration though. One alternative is using void*
>>> as
>>>
>> the
>>
>>> > type but that violates type safety and would require static_casts.
>>> wdyt?
>>>
>>
>> It would just make it worse (hiding the circular dependency, and as you
>>>
>> mention,
>>
>>> violating the type safety).
>>>
>>
>> Maybe what you want is to pass a GLStateRestorer to RestoreState, and do
>>> the
>>> GetContextState dance (using internal APIs) in the implementation in the
>>> gpu/
>>> code.
>>>
>>
>> I tried that but eventually you need a dynamic_cast to
>> GLStateRestorerImpl which
>> is disallowed.
>>
>
> Why do you need dynamic_cast? There's only one implementation of
> GLStateRestorer. Why couldn't you static_cast? We wouldn't mix/match
> different implementations anyway (I don't think it could make sense).
>
>
>>
>> How about changing the GLStateRestorer::GetContextState to return a
>> gfx::ContextStateBase* interface (i.e. pure virtual base class) and change
>> gpu::gles2::ContextState to implement that interface. The interface would
>> have
>> methods to query texture ids
>> bound to various targets given a texture unit id that would be called from
>> ContextState::RestoreTextureUnitBindings.
>>
>
> We could, but I don't think that's needed.
>
>
>>
>>
>> https://codereview.chromium.org/118203002/
>>
>
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On Mon, Jan 13, 2014 at 5:29 PM, Kaan Baloglu <kaanb@chromium.org> wrote: > When I use static_cast<> like this, the DCHECK in GetContextState fails. > Is the decoder_ field being set to NULL when we perform the static_cast<>? > > void GLStateRestorerImpl::RestoreState(const gfx::GLStateRestorer* > restorer) { > DCHECK(decoder_.get()); > const GLStateRestorerImpl* restorer_impl = > static_cast<const GLStateRestorerImpl*>(restorer); > decoder_->RestoreState(restorer_impl->GetContextState()); > } > static_cast has no effect on the pointed-to object, it doesn't even dereference it. It only (at best) adjusts the pointer value (and I don't think it'd do it in this case). IOW I don't know why you'd be hitting the assert if the rest of the code is otherwise correct. Antoine > > > > > On Mon, Jan 13, 2014 at 4:32 PM, Antoine Labour <piman@chromium.org>wrote: > >> >> >> >> On Mon, Jan 13, 2014 at 4:14 PM, <kaanb@chromium.org> wrote: >> >>> On 2014/01/13 22:37:46, piman wrote: >>> >>> https://codereview.chromium.org/118203002/diff/400001/ui/ >>> gl/gl_state_restorer.h >>> >>>> File ui/gl/gl_state_restorer.h (right): >>>> >>> >>> >>> https://codereview.chromium.org/118203002/diff/400001/ui/ >>> gl/gl_state_restorer.h#newcode30 >>> >>>> ui/gl/gl_state_restorer.h:30: virtual const gpu::gles2::ContextState* >>>> GetContextState() = 0; >>>> On 2014/01/13 22:17:00, kaanb wrote: >>>> > On 2014/01/13 22:12:46, piman wrote: >>>> > > Mmh, this causes a dependency cycle gpu -> ui -> gpu >>>> > >>>> > It's only a forward declaration though. One alternative is using >>>> void* as >>>> >>> the >>> >>>> > type but that violates type safety and would require static_casts. >>>> wdyt? >>>> >>> >>> It would just make it worse (hiding the circular dependency, and as you >>>> >>> mention, >>> >>>> violating the type safety). >>>> >>> >>> Maybe what you want is to pass a GLStateRestorer to RestoreState, and >>>> do the >>>> GetContextState dance (using internal APIs) in the implementation in >>>> the gpu/ >>>> code. >>>> >>> >>> I tried that but eventually you need a dynamic_cast to >>> GLStateRestorerImpl which >>> is disallowed. >>> >> >> Why do you need dynamic_cast? There's only one implementation of >> GLStateRestorer. Why couldn't you static_cast? We wouldn't mix/match >> different implementations anyway (I don't think it could make sense). >> >> >>> >>> How about changing the GLStateRestorer::GetContextState to return a >>> gfx::ContextStateBase* interface (i.e. pure virtual base class) and >>> change >>> gpu::gles2::ContextState to implement that interface. The interface >>> would have >>> methods to query texture ids >>> bound to various targets given a texture unit id that would be called >>> from >>> ContextState::RestoreTextureUnitBindings. >>> >> >> We could, but I don't think that's needed. >> >> >>> >>> >>> https://codereview.chromium.org/118203002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Checking for GLStateRestorer* being NULL fixed the crash I was seeing. PTAL. https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8901: SpecializedSetup<ActiveTexture, 0>(true); On 2014/01/13 22:06:29, sievers wrote: > I don't see the specialization for this anywhere (and the default impl is a > NOP). Removed. https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8950: InitializeContextState(&prev_state, 0, 0); On 2014/01/13 22:06:29, sievers wrote: > nit: This would break if kServiceDefaultTexture2dId was 0. > Can we explicitly set it to a valid texture id (for example kServiceTextureId)? Done. https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8988: InitializeContextState(&prev_state, 1, 0); On 2014/01/13 22:06:29, sievers wrote: > nit: here also use a real id (not 0). Done. https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): https://codereview.chromium.org/118203002/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:98: uint32 active_unit) { On 2014/01/13 22:06:29, sievers wrote: > This seems to be very specific to your tests. > How about subclassing GLES2DecoderManualInitTest for your purposes and moving > the added functions from this file to over there? > > Note: You want to make sure the class name starts with 'GLES2Decoder*' to > satisfy existing heap check exclusion filters. Done.
lgtm with nits https://codereview.chromium.org/118203002/diff/620001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): https://codereview.chromium.org/118203002/diff/620001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:17: #include "gpu/command_buffer/service/context_state.h" not needed https://codereview.chromium.org/118203002/diff/620001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h (right): https://codereview.chromium.org/118203002/diff/620001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h:365: ContextState* state, uint32 non_default_unit, uint32 active_unit); remove these https://codereview.chromium.org/118203002/diff/620001/ui/gl/gl_state_restorer.cc File ui/gl/gl_state_restorer.cc (left): https://codereview.chromium.org/118203002/diff/620001/ui/gl/gl_state_restorer... ui/gl/gl_state_restorer.cc:16: nit: whitespace change
LGTM % Daniel's comments.
I also made two minor modifications in addition to the code review feedback. I pass a const GLStateRestorer* instead of non-const and made GLStateRestorerImpl::GetContextState private https://codereview.chromium.org/118203002/diff/620001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): https://codereview.chromium.org/118203002/diff/620001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:17: #include "gpu/command_buffer/service/context_state.h" On 2014/01/14 22:19:28, sievers wrote: > not needed Done. https://codereview.chromium.org/118203002/diff/620001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h (right): https://codereview.chromium.org/118203002/diff/620001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.h:365: ContextState* state, uint32 non_default_unit, uint32 active_unit); On 2014/01/14 22:19:28, sievers wrote: > remove these Done. https://codereview.chromium.org/118203002/diff/620001/ui/gl/gl_state_restorer.cc File ui/gl/gl_state_restorer.cc (left): https://codereview.chromium.org/118203002/diff/620001/ui/gl/gl_state_restorer... ui/gl/gl_state_restorer.cc:16: On 2014/01/14 22:19:28, sievers wrote: > nit: whitespace change Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaanb@chromium.org/118203002/840001
Failed to trigger a try job on win_x64_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaanb@chromium.org/118203002/1080001
I had to change 3 fwd-declaration from class ContextState to struct ContextState. On Wed, Jan 15, 2014 at 8:45 AM, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/kaanb@chromium. > org/118203002/1080001 > > > https://codereview.chromium.org/118203002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Failed to trigger a try job on mac_rel HTTP Error 400: Bad Request
I made another change to return const ContextState* instead of non-const from the mock GetContextState method. On Wed, Jan 15, 2014 at 10:21 AM, <commit-bot@chromium.org> wrote: > Failed to trigger a try job on mac_rel > > HTTP Error 400: Bad Request > > https://codereview.chromium.org/118203002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaanb@chromium.org/118203002/1310001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaanb@chromium.org/118203002/1310001
Message was sent while issue was closed.
Change committed as 245109 |
