Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961633002:
cc: HUD handle context lost in Gpu raster.
Descriptioncc: HUD handle context lost in Gpu raster.
This returns the resource id in case the HUD canvas
is invalid, because of context loss while SkSurface
creation. The resource is subsequently evicted from
quad list.
BUG=736127
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Patch Set 1 #Patch Set 2 : update #
Total comments: 9
Patch Set 3 : update #Patch Set 4 : update #
Total comments: 8
Patch Set 5 : update comments. #
Messages
Total messages: 17 (9 generated)
Description was changed from ========== cc: HUD handle context lost in Gpu raster. BUG=736127 ========== to ========== cc: HUD handle context lost in Gpu raster. BUG=736127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Description was changed from ========== cc: HUD handle context lost in Gpu raster. BUG=736127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: HUD handle context lost in Gpu raster. This returns the resource id to be evicted from quad list in case the HUD canvas is invalid, because of context loss while SkSurface creation. BUG=736127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc: HUD handle context lost in Gpu raster. This returns the resource id to be evicted from quad list in case the HUD canvas is invalid, because of context loss while SkSurface creation. BUG=736127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: HUD handle context lost in Gpu raster. This returns the resource id in case the HUD canvas is invalid, because of context loss while SkSurface creation. The resource is subsequently evicted from quad list. BUG=736127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
sohan.jyoti@huawei.com changed reviewers: + danakj@chromium.org, ericrk@chromium.org, vmpstr@chromium.org
PTAL. Thanks.
On 2017/06/27 12:04:57, sohan wrote: > PTAL. Thanks. Ping! :)
https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:160: uint32_t HeadsUpDisplayLayerImpl::UpdateHudTexture( The actual return type is ResourceId (even though it's just a typedef). I think we should still use that. https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:183: return resources_.back()->id(); Should we be somehow cleaning up resources_ if things are no longer valid? https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.h:46: uint32_t UpdateHudTexture(DrawMode draw_mode, Can you leave a comment on what this returns. https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1704: uint32_t evict_res_id = 0U; ResourceId? also evict_resource_id. https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1741: if (resource_id != evict_res_id) { This doesn't look right. The resource should not be in the quad list if it's evicted. Maybe check this during HUDLayerImpl::AppendQuads instead and keep all of the logic limited to that class?
PTAL. Thanks. https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:160: uint32_t HeadsUpDisplayLayerImpl::UpdateHudTexture( On 2017/06/30 23:16:51, vmpstr wrote: > The actual return type is ResourceId (even though it's just a typedef). I think > we should still use that. Done. https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:183: return resources_.back()->id(); On 2017/06/30 23:16:51, vmpstr wrote: > Should we be somehow cleaning up resources_ if things are no longer valid? Hmm. we are in ScopedWriteLockGL scope here, so if we delete the ScopedResource from vector it will free itself and dcheck for lock for write in ResourceProvider::DeleteResource. Wouldnt scope take care of it implicitly ? https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1704: uint32_t evict_res_id = 0U; On 2017/06/30 23:16:51, vmpstr wrote: > ResourceId? also evict_resource_id. Done. https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1741: if (resource_id != evict_res_id) { On 2017/06/30 23:16:51, vmpstr wrote: > This doesn't look right. > > The resource should not be in the quad list if it's evicted. Maybe check this > during HUDLayerImpl::AppendQuads instead and keep all of the logic limited to > that class? Yeah this is kinda odd. The problem is for HUD layers, append quad happens before sksurface creation (in UpdateHudTexture for ganesh backend) and if the context loss happens during surface creation, we are stuck with the resource in quad list. We can of course traverse the renderpass and evict, but thought it is more efficient to avoid passing to resource array here. wdyt ?
On 2017/07/04 16:51:13, sohan wrote: > PTAL. Thanks. > > https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... > File cc/layers/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... > cc/layers/heads_up_display_layer_impl.cc:160: uint32_t > HeadsUpDisplayLayerImpl::UpdateHudTexture( > On 2017/06/30 23:16:51, vmpstr wrote: > > The actual return type is ResourceId (even though it's just a typedef). I > think > > we should still use that. > > Done. > > https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... > cc/layers/heads_up_display_layer_impl.cc:183: return resources_.back()->id(); > On 2017/06/30 23:16:51, vmpstr wrote: > > Should we be somehow cleaning up resources_ if things are no longer valid? > > Hmm. we are in ScopedWriteLockGL scope here, so if we delete the ScopedResource > from vector it will free itself and dcheck for lock for write in > ResourceProvider::DeleteResource. > Wouldnt scope take care of it implicitly ? > > https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:1704: uint32_t evict_res_id = 0U; > On 2017/06/30 23:16:51, vmpstr wrote: > > ResourceId? also evict_resource_id. > > Done. > > https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:1741: if (resource_id != evict_res_id) { > On 2017/06/30 23:16:51, vmpstr wrote: > > This doesn't look right. > > > > The resource should not be in the quad list if it's evicted. Maybe check this > > during HUDLayerImpl::AppendQuads instead and keep all of the logic limited to > > that class? > > Yeah this is kinda odd. > The problem is for HUD layers, append quad happens before sksurface creation (in > UpdateHudTexture for ganesh backend) and if the context loss happens during > surface creation, we are stuck with the resource in quad list. > We can of course traverse the renderpass and evict, but thought it is more > efficient to avoid passing to resource array here. wdyt ? The problem is that this is the hud layer, which is used infrequently and mostly for debugging. So, I think I would prefer doing extra work that is limited to just the hud layer as opposed to having an extra if exposed in code that is always run. Does that make sense? I still would prefer to make this hidden inside the hud layer. Do you think that's possible (even if it means an extra iteration there)?
Patchset #4 (id:120001) has been deleted
On 2017/07/11 16:42:56, vmpstr wrote: > On 2017/07/04 16:51:13, sohan wrote: > > PTAL. Thanks. > > > > > https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... > > File cc/layers/heads_up_display_layer_impl.cc (right): > > > > > https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... > > cc/layers/heads_up_display_layer_impl.cc:160: uint32_t > > HeadsUpDisplayLayerImpl::UpdateHudTexture( > > On 2017/06/30 23:16:51, vmpstr wrote: > > > The actual return type is ResourceId (even though it's just a typedef). I > > think > > > we should still use that. > > > > Done. > > > > > https://codereview.chromium.org/2961633002/diff/80001/cc/layers/heads_up_disp... > > cc/layers/heads_up_display_layer_impl.cc:183: return resources_.back()->id(); > > On 2017/06/30 23:16:51, vmpstr wrote: > > > Should we be somehow cleaning up resources_ if things are no longer valid? > > > > Hmm. we are in ScopedWriteLockGL scope here, so if we delete the > ScopedResource > > from vector it will free itself and dcheck for lock for write in > > ResourceProvider::DeleteResource. > > Wouldnt scope take care of it implicitly ? > > > > > https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... > > File cc/trees/layer_tree_host_impl.cc (right): > > > > > https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... > > cc/trees/layer_tree_host_impl.cc:1704: uint32_t evict_res_id = 0U; > > On 2017/06/30 23:16:51, vmpstr wrote: > > > ResourceId? also evict_resource_id. > > > > Done. > > > > > https://codereview.chromium.org/2961633002/diff/80001/cc/trees/layer_tree_hos... > > cc/trees/layer_tree_host_impl.cc:1741: if (resource_id != evict_res_id) { > > On 2017/06/30 23:16:51, vmpstr wrote: > > > This doesn't look right. > > > > > > The resource should not be in the quad list if it's evicted. Maybe check > this > > > during HUDLayerImpl::AppendQuads instead and keep all of the logic limited > to > > > that class? > > > > Yeah this is kinda odd. > > The problem is for HUD layers, append quad happens before sksurface creation > (in > > UpdateHudTexture for ganesh backend) and if the context loss happens during > > surface creation, we are stuck with the resource in quad list. > > We can of course traverse the renderpass and evict, but thought it is more > > efficient to avoid passing to resource array here. wdyt ? > > The problem is that this is the hud layer, which is used infrequently and mostly > for debugging. So, I think I would prefer doing extra work that is limited to > just the hud layer as opposed to having an extra if exposed in code that is > always run. Does that make sense? I still would prefer to make this hidden > inside the hud layer. Do you think that's possible (even if it means an extra > iteration there)? Done, PTAL. Thanks. F
This is looking much better, mostly just request for comments. https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... cc/layers/heads_up_display_layer_impl.cc:827: ResourceId evict_resource_id = resources_.back()->id(); Leave a detailed comment here please (explain 1. what this is doing and 2. why it's doing it) https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... cc/layers/heads_up_display_layer_impl.cc:830: it != render_pass->quad_list.end(); it++) { nit: ++it https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... cc/layers/heads_up_display_layer_impl.h:46: ResourceId UpdateHudTexture(DrawMode draw_mode, Leave a comment on what this returns please. https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... cc/layers/heads_up_display_layer_impl.h:58: void EvictHudQuad(const RenderPassList& list); Leave a comment for new functions please.
PTAL. Thanks. https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... cc/layers/heads_up_display_layer_impl.cc:827: ResourceId evict_resource_id = resources_.back()->id(); On 2017/07/12 23:39:54, vmpstr wrote: > Leave a detailed comment here please (explain 1. what this is doing and 2. why > it's doing it) Done. https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... cc/layers/heads_up_display_layer_impl.cc:830: it != render_pass->quad_list.end(); it++) { On 2017/07/12 23:39:54, vmpstr wrote: > nit: ++it Done. https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... cc/layers/heads_up_display_layer_impl.h:46: ResourceId UpdateHudTexture(DrawMode draw_mode, On 2017/07/12 23:39:54, vmpstr wrote: > Leave a comment on what this returns please. Done. https://codereview.chromium.org/2961633002/diff/140001/cc/layers/heads_up_dis... cc/layers/heads_up_display_layer_impl.h:58: void EvictHudQuad(const RenderPassList& list); On 2017/07/12 23:39:54, vmpstr wrote: > Leave a comment for new functions please. Done.
danakj@chromium.org changed reviewers: - danakj@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
