From 90e970a6d2c0f3eebbff23edd2aa74f3a201e6d8 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 29 Jun 2016 16:52:35 -0700 Subject: [PATCH] compositing: Only recomposite after a scroll if layers actually moved, and fix the jerky scrolling "pops" during flings on Mac. See the comments in compositor.rs for more details. Requires servo/webrender#302 and servo/webrender_traits#64. --- components/compositing/compositor.rs | 101 ++++++++++++++++---- components/compositing/compositor_thread.rs | 4 + components/servo/Cargo.lock | 4 +- ports/cef/Cargo.lock | 4 +- 4 files changed, 90 insertions(+), 23 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 256612f752f0..92283558b9d4 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -220,6 +220,9 @@ pub struct IOCompositor { /// Pending scroll/zoom events. pending_scroll_zoom_events: Vec, + /// Whether we're waiting on a recomposite after dispatching a scroll. + waiting_for_results_of_scroll: bool, + /// Used by the logic that determines when it is safe to output an /// image for the reftest framework. ready_to_save_state: ReadyState, @@ -254,6 +257,8 @@ struct ScrollZoomEvent { cursor: TypedPoint2D, /// The scroll event phase. phase: ScrollEventPhase, + /// The number of OS events that have been coalesced together into this one event. + event_count: u32, } #[derive(PartialEq, Debug)] @@ -398,6 +403,10 @@ impl webrender_traits::RenderNotifier for RenderNotifier { self.compositor_proxy.recomposite(CompositingReason::NewWebRenderFrame); } + fn new_scroll_frame_ready(&mut self, composite_needed: bool) { + self.compositor_proxy.send(Msg::NewScrollFrameReady(composite_needed)); + } + fn pipeline_size_changed(&mut self, pipeline_id: webrender_traits::PipelineId, size: Option>) { @@ -468,6 +477,7 @@ impl IOCompositor { composition_request: CompositionRequest::NoCompositingNecessary, touch_handler: TouchHandler::new(), pending_scroll_zoom_events: Vec::new(), + waiting_for_results_of_scroll: false, composite_target: composite_target, shutdown_state: ShutdownState::NotShuttingDown, page_zoom: ScaleFactor::new(1.0), @@ -677,7 +687,6 @@ impl IOCompositor { } (Msg::DelayedCompositionTimeout(timestamp), ShutdownState::NotShuttingDown) => { - debug!("delayed composition timeout!"); if let CompositionRequest::DelayedComposite(this_timestamp) = self.composition_request { if timestamp == this_timestamp { @@ -789,6 +798,14 @@ impl IOCompositor { } } + (Msg::NewScrollFrameReady(recomposite_needed), ShutdownState::NotShuttingDown) => { + self.waiting_for_results_of_scroll = false; + if recomposite_needed { + self.composition_request = CompositionRequest::CompositeNow( + CompositingReason::NewWebRenderScrollFrame); + } + } + // When we are shutting_down, we need to avoid performing operations // such as Paint that may crash because we have begun tearing down // the rest of our resources. @@ -1186,8 +1203,8 @@ impl IOCompositor { fn schedule_delayed_composite_if_necessary(&mut self) { match self.composition_request { - CompositionRequest::CompositeNow(_) | - CompositionRequest::DelayedComposite(_) => return, + CompositionRequest::CompositeNow(_) => return, + CompositionRequest::DelayedComposite(_) | CompositionRequest::NoCompositingNecessary => {} } @@ -1509,6 +1526,7 @@ impl IOCompositor { delta: scroll_delta, cursor: cursor, phase: ScrollEventPhase::Move(true), + event_count: 1, }); self.composite_if_necessary_if_not_using_webrender(CompositingReason::Zoom); } @@ -1563,6 +1581,7 @@ impl IOCompositor { delta: delta, cursor: cursor, phase: ScrollEventPhase::Move(self.scroll_in_progress), + event_count: 1, }); self.composite_if_necessary_if_not_using_webrender(CompositingReason::Scroll); } @@ -1576,6 +1595,7 @@ impl IOCompositor { delta: delta, cursor: cursor, phase: ScrollEventPhase::Start, + event_count: 1, }); self.composite_if_necessary_if_not_using_webrender(CompositingReason::Scroll); } @@ -1589,6 +1609,7 @@ impl IOCompositor { delta: delta, cursor: cursor, phase: ScrollEventPhase::End, + event_count: 1, }); self.composite_if_necessary_if_not_using_webrender(CompositingReason::Scroll); } @@ -1605,35 +1626,52 @@ impl IOCompositor { let this_cursor = scroll_event.cursor; if let Some(combined_event) = last_combined_event { if combined_event.phase != scroll_event.phase { - webrender_api.scroll( - (combined_event.delta / self.scene.scale).to_untyped(), - (combined_event.cursor.as_f32() / self.scene.scale).to_untyped(), - combined_event.phase); + let delta = (combined_event.delta / self.scene.scale).to_untyped(); + let cursor = (combined_event.cursor.as_f32() / + self.scene.scale).to_untyped(); + webrender_api.scroll(delta, cursor, combined_event.phase); last_combined_event = None } } - match last_combined_event { - None => { - last_combined_event = Some(ScrollZoomEvent { + match (&mut last_combined_event, scroll_event.phase) { + (last_combined_event @ &mut None, _) => { + *last_combined_event = Some(ScrollZoomEvent { magnification: scroll_event.magnification, delta: this_delta, cursor: this_cursor, phase: scroll_event.phase, + event_count: 1, }) } - Some(ref mut last_combined_event) => { + (&mut Some(ref mut last_combined_event), + ScrollEventPhase::Move(false)) => { + // Mac OS X sometimes delivers scroll events out of vsync during a + // fling. This causes events to get bunched up occasionally, causing + // nasty-looking "pops". To mitigate this, during a fling we average + // deltas instead of summing them. + let old_event_count = + ScaleFactor::new(last_combined_event.event_count as f32); + last_combined_event.event_count += 1; + let new_event_count = + ScaleFactor::new(last_combined_event.event_count as f32); + last_combined_event.delta = + (last_combined_event.delta * old_event_count + this_delta) / + new_event_count; + } + (&mut Some(ref mut last_combined_event), _) => { last_combined_event.delta = last_combined_event.delta + this_delta; + last_combined_event.event_count += 1 } } } // TODO(gw): Support zoom (WR issue #28). if let Some(combined_event) = last_combined_event { - webrender_api.scroll( - (combined_event.delta / self.scene.scale).to_untyped(), - (combined_event.cursor.as_f32() / self.scene.scale).to_untyped(), - combined_event.phase); + let delta = (combined_event.delta / self.scene.scale).to_untyped(); + let cursor = (combined_event.cursor.as_f32() / self.scene.scale).to_untyped(); + webrender_api.scroll(delta, cursor, combined_event.phase); + self.waiting_for_results_of_scroll = true } } None => { @@ -1815,6 +1853,7 @@ impl IOCompositor { delta: Point2D::typed(0.0, 0.0), // TODO: Scroll to keep the center in view? cursor: Point2D::typed(-1, -1), // Make sure this hits the base layer. phase: ScrollEventPhase::Move(true), + event_count: 1, }); self.composite_if_necessary_if_not_using_webrender(CompositingReason::Zoom); } @@ -2281,6 +2320,7 @@ impl IOCompositor { self.process_animations(); self.start_scrolling_bounce_if_necessary(); + self.waiting_for_results_of_scroll = false; Ok(rv) } @@ -2515,7 +2555,27 @@ impl IOCompositor { pub fn handle_events(&mut self, messages: Vec) -> bool { // Check for new messages coming from the other threads in the system. + let mut compositor_messages = vec![]; + let mut found_recomposite_msg = false; while let Some(msg) = self.port.try_recv_compositor_msg() { + match msg { + Msg::Recomposite(_) if found_recomposite_msg => {} + Msg::Recomposite(_) => { + found_recomposite_msg = true; + compositor_messages.push(msg) + } + _ => compositor_messages.push(msg), + } + } + if found_recomposite_msg { + compositor_messages.retain(|msg| { + match *msg { + Msg::DelayedCompositionTimeout(_) => false, + _ => true, + } + }) + } + for msg in compositor_messages { if !self.handle_browser_message(msg) { break } @@ -2537,10 +2597,6 @@ impl IOCompositor { self.send_buffer_requests_for_all_layers(); } - if !self.pending_scroll_zoom_events.is_empty() && opts::get().use_webrender { - self.process_pending_scroll_events() - } - match self.composition_request { CompositionRequest::NoCompositingNecessary | CompositionRequest::DelayedComposite(_) => {} @@ -2549,6 +2605,11 @@ impl IOCompositor { } } + if !self.pending_scroll_zoom_events.is_empty() && !self.waiting_for_results_of_scroll && + opts::get().use_webrender { + self.process_pending_scroll_events() + } + self.shutdown_state != ShutdownState::FinishedShuttingDown } @@ -2650,4 +2711,6 @@ pub enum CompositingReason { Zoom, /// A new WebRender frame has arrived. NewWebRenderFrame, + /// WebRender has processed a scroll event and has generated a new frame. + NewWebRenderScrollFrame, } diff --git a/components/compositing/compositor_thread.rs b/components/compositing/compositor_thread.rs index d29235858b8d..24b9caba0a7f 100644 --- a/components/compositing/compositor_thread.rs +++ b/components/compositing/compositor_thread.rs @@ -185,6 +185,9 @@ pub enum Msg { GetScrollOffset(PipelineId, LayerId, IpcSender>), /// Pipeline visibility changed PipelineVisibilityChanged(PipelineId, bool), + /// WebRender has successfully processed a scroll. The boolean specifies whether a composite is + /// needed. + NewScrollFrameReady(bool), /// A pipeline was shut down. // This message acts as a synchronization point between the constellation, // when it shuts down a pipeline, to the compositor; when the compositor @@ -228,6 +231,7 @@ impl Debug for Msg { Msg::PipelineVisibilityChanged(..) => write!(f, "PipelineVisibilityChanged"), Msg::PipelineExited(..) => write!(f, "PipelineExited"), Msg::GetScrollOffset(..) => write!(f, "GetScrollOffset"), + Msg::NewScrollFrameReady(..) => write!(f, "NewScrollFrameReady"), } } } diff --git a/components/servo/Cargo.lock b/components/servo/Cargo.lock index 318f775337d3..d0af1efd0de7 100644 --- a/components/servo/Cargo.lock +++ b/components/servo/Cargo.lock @@ -2585,7 +2585,7 @@ dependencies = [ [[package]] name = "webrender" version = "0.1.0" -source = "git+https://github.com/servo/webrender#85f887b21f6eac3e33aea3fee0f076dc7c890307" +source = "git+https://github.com/servo/webrender#f69f9a1a4cf31c60dc03480abfd0a74aeb305f9c" dependencies = [ "app_units 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", "bit-set 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -2609,7 +2609,7 @@ dependencies = [ [[package]] name = "webrender_traits" version = "0.2.0" -source = "git+https://github.com/servo/webrender_traits#a466592568193d9ec8a57de9837dddb28ee9631e" +source = "git+https://github.com/servo/webrender_traits#d86e51ace4fd1b43123e0490dc80f631be0726d0" dependencies = [ "app_units 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 0.5.3 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/ports/cef/Cargo.lock b/ports/cef/Cargo.lock index acdd3727e8ba..2512642e7245 100644 --- a/ports/cef/Cargo.lock +++ b/ports/cef/Cargo.lock @@ -2447,7 +2447,7 @@ dependencies = [ [[package]] name = "webrender" version = "0.1.0" -source = "git+https://github.com/servo/webrender#85f887b21f6eac3e33aea3fee0f076dc7c890307" +source = "git+https://github.com/servo/webrender#f69f9a1a4cf31c60dc03480abfd0a74aeb305f9c" dependencies = [ "app_units 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", "bit-set 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -2471,7 +2471,7 @@ dependencies = [ [[package]] name = "webrender_traits" version = "0.2.0" -source = "git+https://github.com/servo/webrender_traits#a466592568193d9ec8a57de9837dddb28ee9631e" +source = "git+https://github.com/servo/webrender_traits#d86e51ace4fd1b43123e0490dc80f631be0726d0" dependencies = [ "app_units 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 0.5.3 (registry+https://github.com/rust-lang/crates.io-index)",