From c1b0a9eb936cbefa6717164a5f18c4005d1447c0 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 7 Jul 2014 16:16:42 -0700 Subject: [PATCH 1/8] Add a new find_layer_with_pipeline_and_layer_id method This method will be used to eliminate a lot of the ad-hoc recursive searches that are present in the compositor layer code. --- src/components/compositing/compositor_data.rs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/components/compositing/compositor_data.rs b/src/components/compositing/compositor_data.rs index e0965aab059..61b9298dc90 100644 --- a/src/components/compositing/compositor_data.rs +++ b/src/components/compositing/compositor_data.rs @@ -574,6 +574,27 @@ impl CompositorData { return None } + pub fn find_layer_with_pipeline_and_layer_id(layer: Rc>, + pipeline_id: PipelineId, + layer_id: LayerId) + -> Option>> { + if layer.extra_data.borrow().pipeline.id == pipeline_id && + layer.extra_data.borrow().id == layer_id { + return Some(layer.clone()); + } + + for kid in layer.children().iter() { + match CompositorData::find_layer_with_pipeline_and_layer_id(kid.clone(), + pipeline_id, + layer_id) { + v @ Some(_) => { return v; } + None => { } + } + } + + return None; + } + // A helper method to resize sublayers. fn resize_helper(layer: Rc>, pipeline_id: PipelineId, From 0396cdb1c5e721bc7ff9bc26e15770effcf9c61c Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 7 Jul 2014 16:16:42 -0700 Subject: [PATCH 2/8] Rename find_child_with_layer_and_pipeline_id Rename find_child_with_layer_and_pipeline_id to find_child_with_pipeline_and_layer_id so that it matches both the logical and actual parameter ordering. --- src/components/compositing/compositor_data.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/compositing/compositor_data.rs b/src/components/compositing/compositor_data.rs index 61b9298dc90..66f8cde0754 100644 --- a/src/components/compositing/compositor_data.rs +++ b/src/components/compositing/compositor_data.rs @@ -409,7 +409,7 @@ impl CompositorData { new_rect: Rect) -> bool { debug!("compositor_data: starting set_clipping_rect()"); - match CompositorData::find_child_with_layer_and_pipeline_id(layer.clone(), + match CompositorData::find_child_with_pipeline_and_layer_id(layer.clone(), pipeline_id, layer_id) { Some(child_node) => { @@ -561,7 +561,7 @@ impl CompositorData { - fn find_child_with_layer_and_pipeline_id(layer: Rc>, + fn find_child_with_pipeline_and_layer_id(layer: Rc>, pipeline_id: PipelineId, layer_id: LayerId) -> Option>> { @@ -604,7 +604,7 @@ impl CompositorData { -> bool { debug!("compositor_data: starting resize_helper()"); - let found = match CompositorData::find_child_with_layer_and_pipeline_id(layer.clone(), + let found = match CompositorData::find_child_with_pipeline_and_layer_id(layer.clone(), pipeline_id, layer_id) { Some(child) => { From f91d51cb2c700456bbc2b9c1c5c81cbbcf6a58c7 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 7 Jul 2014 16:16:42 -0700 Subject: [PATCH 3/8] Eliminate ad-hoc recursion in set_unrendered_color --- src/components/compositing/compositor.rs | 16 +++++++++------ src/components/compositing/compositor_data.rs | 20 +------------------ 2 files changed, 11 insertions(+), 25 deletions(-) diff --git a/src/components/compositing/compositor.rs b/src/components/compositing/compositor.rs index 72dc10c3209..31c9809ec25 100644 --- a/src/components/compositing/compositor.rs +++ b/src/components/compositing/compositor.rs @@ -348,12 +348,16 @@ impl IOCompositor { fn set_unrendered_color(&mut self, pipeline_id: PipelineId, layer_id: LayerId, color: Color) { match self.scene.root { - Some(ref layer) => CompositorData::set_unrendered_color(layer.clone(), - pipeline_id, - layer_id, - color), - None => false, - }; + Some(ref root_layer) => { + match CompositorData::find_layer_with_pipeline_and_layer_id(root_layer.clone(), + pipeline_id, + layer_id) { + Some(ref layer) => CompositorData::set_unrendered_color(layer.clone(), color), + None => { } + } + } + None => { } + } } fn set_ids(&mut self, diff --git a/src/components/compositing/compositor_data.rs b/src/components/compositing/compositor_data.rs index 66f8cde0754..5712efc9e34 100644 --- a/src/components/compositing/compositor_data.rs +++ b/src/components/compositing/compositor_data.rs @@ -813,26 +813,8 @@ impl CompositorData { } } - pub fn set_unrendered_color(layer: Rc>, - pipeline_id: PipelineId, - layer_id: LayerId, - color: Color) - -> bool { - if layer.extra_data.borrow().pipeline.id != pipeline_id || - layer.extra_data.borrow().id != layer_id { - for child_layer in layer.children().iter() { - if CompositorData::set_unrendered_color(child_layer.clone(), - pipeline_id, - layer_id, - color) { - return true; - } - } - return false; - } - + pub fn set_unrendered_color(layer: Rc>, color: Color) { layer.extra_data.borrow_mut().unrendered_color = color; - return true; } } From 512c986915be4af71f0fec8d7600ef57adca1a80 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 7 Jul 2014 16:16:42 -0700 Subject: [PATCH 4/8] Eliminate iteration over Option types This idiom has been dying out in new Rust code; it's especially confusing for layers because there is a higher chance of making the mistaken assumption that the iteration is occurring over multiple layers. --- src/components/compositing/compositor.rs | 39 +++++++++++++++--------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/src/components/compositing/compositor.rs b/src/components/compositing/compositor.rs index 31c9809ec25..0e35ce30498 100644 --- a/src/components/compositing/compositor.rs +++ b/src/components/compositing/compositor.rs @@ -413,8 +413,9 @@ impl IOCompositor { Scrollable)); // Release all tiles from the layer before dropping it. - for layer in self.scene.root.mut_iter() { - CompositorData::clear_all_tiles(layer.clone()); + match self.scene.root { + Some(ref mut layer) => CompositorData::clear_all_tiles(layer.clone()), + None => { } } self.scene.root = Some(new_root); } @@ -685,11 +686,14 @@ impl IOCompositor { let page_cursor = cursor.as_f32() / scale; let page_window = self.page_window(); let mut scroll = false; - for layer in self.scene.root.mut_iter() { - scroll = CompositorData::handle_scroll_event(layer.clone(), - page_delta, - page_cursor, - page_window) || scroll; + match self.scene.root { + Some(ref mut layer) => { + scroll = CompositorData::handle_scroll_event(layer.clone(), + page_delta, + page_cursor, + page_window) || scroll; + } + None => { } } self.recomposite_if(scroll); self.ask_for_tiles(); @@ -739,11 +743,14 @@ impl IOCompositor { let page_cursor = TypedPoint2D(-1f32, -1f32); // Make sure this hits the base layer let page_window = self.page_window(); - for layer in self.scene.root.mut_iter() { - CompositorData::handle_scroll_event(layer.clone(), - page_delta, - page_cursor, - page_window); + match self.scene.root { + Some(ref mut layer) => { + CompositorData::handle_scroll_event(layer.clone(), + page_delta, + page_cursor, + page_window); + } + None => { } } self.recomposite = true; @@ -762,17 +769,19 @@ impl IOCompositor { fn ask_for_tiles(&mut self) { let scale = self.device_pixels_per_page_px(); let page_window = self.page_window(); - for layer in self.scene.root.mut_iter() { - if !layer.extra_data.borrow().hidden { + match self.scene.root { + Some(ref mut layer) if !layer.extra_data.borrow().hidden => { let rect = Rect(Point2D(0f32, 0f32), page_window.to_untyped()); let recomposite = CompositorData::get_buffer_request(layer.clone(), &self.graphics_context, rect, scale.get()); self.recomposite = self.recomposite || recomposite; - } else { + } + Some(_) => { debug!("Compositor: root layer is hidden!"); } + None => { } } } From 4c0f8c8ac031ab84635b0ce0fc3586f5f74689a5 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 7 Jul 2014 16:16:42 -0700 Subject: [PATCH 5/8] Rename get_buffer_requests to send_buffer_requests_recursively --- src/components/compositing/compositor.rs | 3 ++- src/components/compositing/compositor_data.rs | 22 +++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/components/compositing/compositor.rs b/src/components/compositing/compositor.rs index 0e35ce30498..2107928a719 100644 --- a/src/components/compositing/compositor.rs +++ b/src/components/compositing/compositor.rs @@ -772,7 +772,8 @@ impl IOCompositor { match self.scene.root { Some(ref mut layer) if !layer.extra_data.borrow().hidden => { let rect = Rect(Point2D(0f32, 0f32), page_window.to_untyped()); - let recomposite = CompositorData::get_buffer_request(layer.clone(), + let recomposite = + CompositorData::send_buffer_requests_recursively(layer.clone(), &self.graphics_context, rect, scale.get()); diff --git a/src/components/compositing/compositor_data.rs b/src/components/compositing/compositor_data.rs index 5712efc9e34..0a72fd955cb 100644 --- a/src/components/compositing/compositor_data.rs +++ b/src/components/compositing/compositor_data.rs @@ -338,11 +338,11 @@ impl CompositorData { // Given the current window size, determine which tiles need to be (re-)rendered and sends them // off the the appropriate renderer. Returns true if and only if the scene should be repainted. - pub fn get_buffer_request(layer: Rc>, - graphics_context: &NativeCompositingGraphicsContext, - window_rect: Rect, - scale: f32) - -> bool { + pub fn send_buffer_requests_recursively(layer: Rc>, + graphics_context: &NativeCompositingGraphicsContext, + window_rect: Rect, + scale: f32) + -> bool { let (request, unused) = Layer::get_tile_rects_page(layer.clone(), window_rect, scale); let redisplay = !unused.is_empty(); if redisplay { @@ -366,7 +366,7 @@ impl CompositorData { CompositorData::build_layer_tree(layer.clone(), graphics_context); } - let get_child_buffer_request = |kid: &Rc>| -> bool { + let send_child_buffer_request = |kid: &Rc>| -> bool { match kid.extra_data.borrow().scissor { Some(scissor) => { let mut new_rect = window_rect; @@ -380,10 +380,10 @@ impl CompositorData { // to make the child_rect appear in coordinates local to it. let child_rect = Rect(new_rect.origin.sub(&scissor.origin), new_rect.size); - CompositorData::get_buffer_request(kid.clone(), - graphics_context, - child_rect, - scale) + CompositorData::send_buffer_requests_recursively(kid.clone(), + graphics_context, + child_rect, + scale) } None => { false // Layer is offscreen @@ -395,7 +395,7 @@ impl CompositorData { }; layer.children().iter().filter(|x| !x.extra_data.borrow().hidden) - .map(get_child_buffer_request) + .map(send_child_buffer_request) .any(|b| b) || redisplay } From c0b599f89455b204c2bf2c6782ca431a658c72fe Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 7 Jul 2014 16:16:42 -0700 Subject: [PATCH 6/8] Remove ad-hoc recursion from add_child_if_necessary --- src/components/compositing/compositor.rs | 40 +++++++++++++------ src/components/compositing/compositor_data.rs | 28 ++----------- 2 files changed, 30 insertions(+), 38 deletions(-) diff --git a/src/components/compositing/compositor.rs b/src/components/compositing/compositor.rs index 2107928a719..1034ec5f07c 100644 --- a/src/components/compositing/compositor.rs +++ b/src/components/compositing/compositor.rs @@ -404,13 +404,20 @@ impl IOCompositor { new_root.extra_data.borrow_mut().unrendered_color = unrendered_color; let parent_layer_id = new_root.extra_data.borrow().id; - assert!(CompositorData::add_child_if_necessary(new_root.clone(), - root_pipeline_id, - parent_layer_id, + match CompositorData::find_layer_with_layer_and_pipeline_id(new_root.clone(), + root_pipeline_id, + parent_layer_id) { + Some(ref mut parent_layer) => { + CompositorData::add_child_if_necessary(parent_layer.clone(), layer_id, Rect(Point2D(0f32, 0f32), size), size, - Scrollable)); + Scrollable); + } + None => { + fail!("Compositor: couldn't find parent layer"); + } + } // Release all tiles from the layer before dropping it. match self.scene.root { @@ -429,19 +436,26 @@ impl IOCompositor { rect: Rect, scroll_policy: ScrollPolicy) { match self.scene.root { - Some(ref root) => { - let parent_layer_id = root.extra_data.borrow().id; - let page_size = root.extra_data.borrow().page_size.unwrap(); - assert!(CompositorData::add_child_if_necessary(root.clone(), - pipeline_id, - parent_layer_id, + Some(ref root_layer) => { + let parent_layer_id = root_layer.extra_data.borrow().id; + match CompositorData::find_layer_with_pipeline_and_layer_id(root_layer.clone(), + pipeline_id, + parent_layer_id) { + Some(ref mut parent_layer) => { + let page_size = root_layer.extra_data.borrow().page_size.unwrap(); + CompositorData::add_child_if_necessary(parent_layer.clone(), layer_id, rect, page_size, - scroll_policy)) + scroll_policy); + } + None => { + fail!("Compositor: couldn't find parent layer"); + } + } } - None => fail!("Compositor: Received new layer without initialized pipeline"), - }; + None => fail!("Compositor: Received new layer without initialized pipeline") + } self.ask_for_tiles(); } diff --git a/src/components/compositing/compositor_data.rs b/src/components/compositing/compositor_data.rs index 0a72fd955cb..ed67fe1584c 100644 --- a/src/components/compositing/compositor_data.rs +++ b/src/components/compositing/compositor_data.rs @@ -146,38 +146,18 @@ impl CompositorData { /// Adds a child layer to the layer with the given ID and the given pipeline, if it doesn't /// exist yet. The child layer will have the same pipeline, tile size, memory limit, and CPU /// painting status as its parent. - /// - /// Returns: - /// * True if the layer was added; - /// * True if the layer was not added because it already existed; - /// * False if the layer could not be added because no suitable parent layer with the given - /// ID and pipeline could be found. pub fn add_child_if_necessary(layer: Rc>, - pipeline_id: PipelineId, - parent_layer_id: LayerId, child_layer_id: LayerId, rect: Rect, page_size: Size2D, - scroll_policy: ScrollPolicy) -> bool { - if layer.extra_data.borrow().pipeline.id != pipeline_id || - layer.extra_data.borrow().id != parent_layer_id { - return layer.children().iter().any(|kid| { - CompositorData::add_child_if_necessary(kid.clone(), - pipeline_id, - parent_layer_id, - child_layer_id, - rect, - page_size, - scroll_policy) - }) - } - + scroll_policy: ScrollPolicy) { // See if we've already made this child layer. + let pipeline_id = layer.extra_data.borrow().pipeline.id; if layer.children().iter().any(|kid| { kid.extra_data.borrow().pipeline.id == pipeline_id && kid.extra_data.borrow().id == child_layer_id }) { - return true + return; } let new_compositor_data = CompositorData::new(layer.extra_data.borrow().pipeline.clone(), @@ -197,8 +177,6 @@ impl CompositorData { // Place the kid's layer in the container passed in. Layer::add_child(layer.clone(), new_kid.clone()); - - true } /// Move the layer's descendants that don't want scroll events and scroll by a relative From 568d4d568a02c23c83c77005f839144fe4ca808f Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 7 Jul 2014 16:16:42 -0700 Subject: [PATCH 7/8] Remove pointless layer lookup We just created the root layer, so there's no point in trying to find it again. --- src/components/compositing/compositor.rs | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/components/compositing/compositor.rs b/src/components/compositing/compositor.rs index 1034ec5f07c..c12d50e1a15 100644 --- a/src/components/compositing/compositor.rs +++ b/src/components/compositing/compositor.rs @@ -394,7 +394,6 @@ impl IOCompositor { }; if layer_id != root_layer_id { - let root_pipeline_id = root_pipeline.id; let new_compositor_data = CompositorData::new_root(root_pipeline, size, self.opts.cpu_painting); @@ -403,21 +402,11 @@ impl IOCompositor { new_compositor_data)); new_root.extra_data.borrow_mut().unrendered_color = unrendered_color; - let parent_layer_id = new_root.extra_data.borrow().id; - match CompositorData::find_layer_with_layer_and_pipeline_id(new_root.clone(), - root_pipeline_id, - parent_layer_id) { - Some(ref mut parent_layer) => { - CompositorData::add_child_if_necessary(parent_layer.clone(), - layer_id, - Rect(Point2D(0f32, 0f32), size), - size, - Scrollable); - } - None => { - fail!("Compositor: couldn't find parent layer"); - } - } + CompositorData::add_child_if_necessary(new_root.clone(), + layer_id, + Rect(Point2D(0f32, 0f32), size), + size, + Scrollable); // Release all tiles from the layer before dropping it. match self.scene.root { From 4b41b68c153811288d444b25d19c0022b4054587 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 7 Jul 2014 16:16:43 -0700 Subject: [PATCH 8/8] Eliminate ad-hoc recursion in add_buffers --- src/components/compositing/compositor.rs | 26 ++++++++++----- src/components/compositing/compositor_data.rs | 32 +++---------------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/src/components/compositing/compositor.rs b/src/components/compositing/compositor.rs index c12d50e1a15..f92a2920531 100644 --- a/src/components/compositing/compositor.rs +++ b/src/components/compositing/compositor.rs @@ -530,14 +530,24 @@ impl IOCompositor { new_layer_buffer_set.mark_will_leak(); match self.scene.root { - Some(ref layer) => { - assert!(CompositorData::add_buffers(layer.clone(), - &self.graphics_context, - pipeline_id, - layer_id, - new_layer_buffer_set, - epoch).is_none()); - self.recomposite = true; + Some(ref root_layer) => { + match CompositorData::find_layer_with_pipeline_and_layer_id(root_layer.clone(), + pipeline_id, + layer_id) { + Some(ref layer) => { + assert!(CompositorData::add_buffers(layer.clone(), + &self.graphics_context, + new_layer_buffer_set, + epoch)); + self.recomposite = true; + } + None => { + // FIXME: This may potentially be triggered by a race condition where a + // buffers are being rendered but the layer is removed before rendering + // completes. + fail!("compositor given paint command for non-existent layer"); + } + } } None => { fail!("compositor given paint command with no root layer initialized"); diff --git a/src/components/compositing/compositor_data.rs b/src/components/compositing/compositor_data.rs index ed67fe1584c..86d4fa2aeb0 100644 --- a/src/components/compositing/compositor_data.rs +++ b/src/components/compositing/compositor_data.rs @@ -675,33 +675,9 @@ impl CompositorData { // layer buffer set is consumed, and None is returned. pub fn add_buffers(layer: Rc>, graphics_context: &NativeCompositingGraphicsContext, - pipeline_id: PipelineId, - layer_id: LayerId, - mut new_buffers: Box, + new_buffers: Box, epoch: Epoch) - -> Option> { - debug!("compositor_data: starting add_buffers()"); - if layer.extra_data.borrow().pipeline.id != pipeline_id || - layer.extra_data.borrow().id != layer_id { - // ID does not match ours, so recurse on descendents (including hidden children). - for child_layer in layer.children().iter() { - match CompositorData::add_buffers(child_layer.clone(), - graphics_context, - pipeline_id, - layer_id, - new_buffers, - epoch) { - None => return None, - Some(buffers) => new_buffers = buffers, - } - } - - // Not found. Give the caller the buffers back. - return Some(new_buffers) - } - - debug!("compositor_data: layers found for add_buffers()"); - + -> bool { if layer.extra_data.borrow().epoch != epoch { debug!("add_buffers: compositor epoch mismatch: {:?} != {:?}, id: {:?}", layer.extra_data.borrow().epoch, @@ -709,7 +685,7 @@ impl CompositorData { layer.extra_data.borrow().pipeline.id); let msg = UnusedBufferMsg(new_buffers.buffers); let _ = layer.extra_data.borrow().pipeline.render_chan.send_opt(msg); - return None + return false; } { @@ -724,7 +700,7 @@ impl CompositorData { } CompositorData::build_layer_tree(layer.clone(), graphics_context); - None + return true; } // Recursively sets occluded portions of quadtrees to Hidden, so that they do not ask for